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

feat(vue)!: refactor vue loader with vue/compiler-sfc #251

Merged
merged 14 commits into from
Dec 13, 2024

Conversation

Teages
Copy link
Contributor

@Teages Teages commented Oct 6, 2024

Refactor the vue loader with vue/compiler-sfc.

Resolve #209 #249 #243 #14 #15
Close #210

Some break changes:

  • Since there is no effective way to confirm the position of blocks, all blocks will be reordered after build. (unless it can be quick return)
  • The script blocks will look a little (?) different from source if developer uses <script setup lang="*">, but they are still human readable
  • see feat(vue)!: refactor vue loader with vue/compiler-sfc #251 (comment)

But I think it is worth.

also updated test.


It is a draft pull request because I found some times vue-tsc^2.1.0 will add the vue global declare in dts output for no resaon.

It happens in my own package Teages/mkdist-vue-loader but after I merged back to mkdist it disappeared....

fixed with 31e93f0

tested with 2452db9

@Teages Teages changed the title feat(vue): new vue loader with vue/compiler-sfc feat(vue): refactor vue loader with vue/compiler-sfc Oct 6, 2024
Copy link

codecov bot commented Oct 6, 2024

Codecov Report

Attention: Patch coverage is 96.57534% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.04%. Comparing base (9000888) to head (d3a30be).
Report is 67 commits behind head on main.

Files with missing lines Patch % Lines
src/loaders/vue.ts 96.15% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
- Coverage   82.86%   81.04%   -1.82%     
==========================================
  Files          12       12              
  Lines         852      881      +29     
  Branches      133      189      +56     
==========================================
+ Hits          706      714       +8     
- Misses        144      165      +21     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Teages
Copy link
Contributor Author

Teages commented Oct 6, 2024

according to tests in 2452db9 I think global declare pollution may be a configuration issue (and should not be handled in this pr), so I think this PR is ready for review.

now fixed

@typed-sigterm
Copy link

@Teages This is a nice workaround! Since the PR hasn't received reviews for 1 month, can you temporarily release this fork to NPM?

Teages added a commit to Teages/mkdist that referenced this pull request Nov 2, 2024
@Teages
Copy link
Contributor Author

Teages commented Nov 2, 2024

@Teages This is a nice workaround! Since the PR hasn't received reviews for 1 month, can you temporarily release this fork to NPM?

@typed-sigterm the package is avalibale on @teages/[email protected]

{
  "devDependencies": {
    "mkdist": "npm:@teages/[email protected]"
  }
}

or:

{
  "resolutions": {
    "mkdist": "npm:@teages/[email protected]",
  }
}

It works with the main branch of nuxt/ui, but fell free to feedback here if you got something goes wrong

I rewrite one of their components with `<script setup>`

image

Teages added a commit to Teages/mkdist that referenced this pull request Dec 9, 2024
@danielroe danielroe changed the title feat(vue): refactor vue loader with vue/compiler-sfc feat(vue)!: refactor vue loader with vue/compiler-sfc Dec 12, 2024
src/loaders/vue.ts Outdated Show resolved Hide resolved
Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

sorry for the long delay in reviewing, but having tested this with the nuxt/module-builder, I think it's a very nice change.

there are some improvements we might need to make but as long as this results in a major bump, we should be fine.

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Great! Thank you ❤️

@Teages
Copy link
Contributor Author

Teages commented Dec 13, 2024

https://github.com/nuxt/module-builder/pull/444/files#diff-a0c83a2b6af95030d68d0ca70d7d56eb24bd106c34c70d6052ed66e05e53b68dR13

export default _default;
declare module 'vue' {
    interface GlobalComponents {
    }
    interface GlobalDirectives {
    }
}
// and many lines...

@danielroe Are these global type export expected? I have removed them in 31e93f0

If the next major release isn't coming soon, I think I should cherry-pick this commit to another pull request.

@danielroe
Copy link
Member

@Teages I added those so we can track the changes in this pr. I didn't see an issue with your changes...

@danielroe danielroe merged commit 52087e8 into unjs:main Dec 13, 2024
4 checks passed
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.

Improve Vue <script setup lang="ts"> handling
3 participants