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: fixed an error in the vue component editor #10293

Merged
merged 3 commits into from
Mar 5, 2024
Merged

fix: fixed an error in the vue component editor #10293

merged 3 commits into from
Mar 5, 2024

Conversation

phk422
Copy link
Contributor

@phk422 phk422 commented Mar 1, 2024

fix withastro/language-tools#758

Changes

  • Modify the regular expression for matching defineProps
  • Removes the condition where comments exist when working with code
  • Fixed an issue with 'defineProps' import error

Copy link

changeset-bot bot commented Mar 1, 2024

⚠️ No Changeset found

Latest commit: 80c7fda

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added pkg: vue Related to Vue (scope) pkg: integration Related to any renderer integration (scope) labels Mar 1, 2024
@phk422
Copy link
Contributor Author

phk422 commented Mar 1, 2024

@ematipico, I apologize for any inconvenience. Due to some unexpected issues, I accidentally closed this PR(#10283 ). I have since resubmitted it, and in doing so, all the issues have been resolved. I appreciate your understanding.

@phk422
Copy link
Contributor Author

phk422 commented Mar 3, 2024

Hi @ematipico! Could you please review the code again? If everything is fine, could you merge it? I would like to resolve this issue as soon as possible.

@ematipico
Copy link
Member

@bluwy may I have a review from you too?

@bluwy bluwy changed the title fix: fixed an error in the vue component editor(https://github.com/wi… fix: fixed an error in the vue component edito Mar 4, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Left a couple comments and the idea seems fine, but I think @Princesseuh has better insights on the tooling side of changes 😅

import { defineProps } from '@vue/runtime-core';
import { defineProps } from 'vue';
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason @vue/runtime-core is changed to vue. We should also remove the indent changes

Copy link
Member

Choose a reason for hiding this comment

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

Would love to know this as well, I remember purposely using @vue/runtime-core instead of vue. IIRC it's not quite the same function. (though now I do wonder if this worked on pnpm since it's not a direct dep?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my case with pnpm, I found that @vue/runtime-core might not be installed, In order to ensure the correct import of defineProps, I changed it to vue because it will be installed.

Copy link
Contributor Author

@phk422 phk422 Mar 5, 2024

Choose a reason for hiding this comment

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

Would love to know this as well, I remember purposely using @vue/runtime-core instead of vue. IIRC it's not quite the same function. (though now I do wonder if this worked on pnpm since it's not a direct dep?)

I tested it with npm and pnpm, just as you thought, it's not directly dependent on pnpm.

packages/integrations/vue/test/toTsx.test.cts Outdated Show resolved Hide resolved
packages/integrations/vue/test/toTsx.test.cts Outdated Show resolved Hide resolved
@bluwy bluwy changed the title fix: fixed an error in the vue component edito fix: fixed an error in the vue component editor Mar 4, 2024
@phk422 phk422 requested a review from bluwy March 5, 2024 02:44
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you @phk422 for the fix 🙏

@ematipico ematipico merged commit 9e76abc into withastro:main Mar 5, 2024
13 checks passed
peng added a commit to peng/astro that referenced this pull request Mar 6, 2024
* main:
  [ci] release (withastro#10337)
  Revert bad release (withastro#10336)
  [ci] release (withastro#10332)
  add missing changeset (withastro#10335)
  [ci] set `--tag` on release (withastro#10323)
  [ci] format
  add back data loss confirmation handling (withastro#10330)
  [ci] format
  fix(rendering): allow render instructions to propagate while rendering slots (withastro#10316)
  [ci] format
  fix: fixed an error in the vue component editor (withastro#10293)
  chore(vercel): update @vercel/nft (withastro#10305)
  Update turbo test dependsOn (withastro#10329)
  Add minimal @astrojs/db readme (withastro#10331)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: vue Related to Vue (scope)
Projects
None yet
4 participants