Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: issues for vue composition api #1558

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nmerget
Copy link
Contributor

@nmerget nmerget commented Sep 9, 2024

Description

Please provide the following information:

There were some issues with vue option.api="composition":

Make sure to follow the PR preparation steps in CONTRIBUTING.md before submitting your PR:

  • format the codebase: from the root, run yarn fmt:prettier.
  • update all snapshots (in core & CLI): from the root, run yarn test:update
  • add Changeset entry: from the root, run yarn g:changeset and follow the CLI instructions. Alternatively, use the Changeset Github Bot to create the file.

Copy link

changeset-bot bot commented Sep 9, 2024

🦋 Changeset detected

Latest commit: 6c6d479

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@builder.io/mitosis Patch
@builder.io/mitosis-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

nx-cloud bot commented Sep 9, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6c6d479. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


🟥 Failed Commands
nx run-many --target test --verbose
✅ Successfully ran 3 targets

Sent with 💌 from NxCloud.

Copy link
Contributor

@samijaber samijaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your replaceNodes call isn't working properly. 🤔

might want to add some unit tests in https://github.com/BuilderIO/mitosis/blob/main/packages/core/src/helpers/replace-identifiers.test.ts

to figure out why that is.

packages/core/src/generators/vue/compositionApi.ts Outdated Show resolved Hide resolved
Comment on lines 54 to 57
valueMapper: (code, _, typeParameter) => {
const cleanCode = code.replaceAll('this.', ''); // Composition api isn't a class we don't need "this." here
return isTs && typeParameter ? `ref<${typeParameter}>(${cleanCode})` : `ref(${cleanCode})`;
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
valueMapper: (code, _, typeParameter) => {
const cleanCode = code.replaceAll('this.', ''); // Composition api isn't a class we don't need "this." here
return isTs && typeParameter ? `ref<${typeParameter}>(${cleanCode})` : `ref(${cleanCode})`;
},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry i meant to suggest reverting

Suggested change
valueMapper: (code, _, typeParameter) => {
const cleanCode = code.replaceAll('this.', ''); // Composition api isn't a class we don't need "this." here
return isTs && typeParameter ? `ref<${typeParameter}>(${cleanCode})` : `ref(${cleanCode})`;
},
valueMapper: (code, _, typeParameter) =>
isTs && typeParameter ? `ref<${typeParameter}>(${code})` : `ref(${code})`,

"type": "property",
},
"_messageId": {
"code": "this._id + \\"-message\\"",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't seem like your replaceNodes logic works properly. this should be state.id.

"<script>
let _id = \\"abc\\";

let _messageId = this._id + \\"-message\\";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another indicator that the replaceNodes isn't doing its job

Copy link
Contributor

@samijaber samijaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing 🎉

@@ -220,6 +216,9 @@ export const replaceNodes = ({
};

return babelTransformExpression(code, {
ThisExpression(path) {
searchAndReplace(path);
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌🏽

@samijaber
Copy link
Contributor

Looks like you have some issues with your Solid and Marko tests:
CleanShot 2024-10-02 at 10 46 20@2x
CleanShot 2024-10-02 at 10 47 28@2x

when tests go from throwing to no longer throwing, if yarn test:update doesn't handle this properly, what I tend to do is delete the snapshot file and recreate it altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants