Skip to content

Conversation

@Teages
Copy link
Contributor

@Teages Teages commented Apr 3, 2025

resolves #281 & #289 (comment)
closes #289

The PR did:

  • Transpile vue template if the file have a typescript script block
  • drop the prev vue loader, using custom <script setup> transformer
  • respect original block order

TODO:

  • transform defineModel

After resolving all TODOs, this should not contain any break changes. But I don't have any ideas on it yet.

Now this PR should have no break changes for output behavior.

@codecov
Copy link

codecov bot commented Apr 3, 2025

Codecov Report

Attention: Patch coverage is 88.05970% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.69%. Comparing base (9000888) to head (d746bb6).
Report is 102 commits behind head on main.

Files with missing lines Patch % Lines
src/loaders/vue.ts 86.20% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #300      +/-   ##
==========================================
+ Coverage   82.86%   83.69%   +0.83%     
==========================================
  Files          12       12              
  Lines         852      969     +117     
  Branches      133      201      +68     
==========================================
+ Hits          706      811     +105     
- Misses        144      157      +13     
+ Partials        2        1       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Teages Teages changed the title feat(vue)!: imporve vue loader with minimal transform feat(vue): imporve vue loader with minimal transform Apr 3, 2025
@danielroe danielroe changed the title feat(vue): imporve vue loader with minimal transform feat(vue): use minimal transform for script setup Apr 4, 2025
Comment on lines +360 to +362
<script>
</script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we remove empty script block / all empty block?

@danielroe
Copy link
Member

@Teages with your permission, could I extract this PR to a separate package + add you as a maintainer?

@Teages
Copy link
Contributor Author

Teages commented Apr 4, 2025

@Teages with your permission, could I extract this PR to a separate package + add you as a maintainer?

it is ok

@danielroe
Copy link
Member

Thank you! Now published as v0.0.1 and available at https://github.com/nuxt-contrib/vue-sfc-transformer.

It would be good to add an option to mkdist that allows disabling the current vue loader implementation, and then upstream anyone that wants this better implementation (like @nuxt/module-builder!) can add it.

Later on, in a new major of mkdist or unbuild we will have the option to swap/update to the new implementation.

Does that sound okay to you?

@Teages
Copy link
Contributor Author

Teages commented Apr 4, 2025

That is fine to me.

It would be good to add an option to mkdist that allows disabling the current vue loader implementation, and then upstream anyone that wants this better implementation (like @nuxt/module-builder!) can add it.

We now have three version:

  • the current one: merge two script block
  • the old one: skip transfrom when the sfc include <script setup>
  • this pr: pre-transform <template> and <script setup>

The first two solutions both have the issue #281. If we list the latter as optional, then we will hardly participate in the transform of vue sfc (only generate the type)

So I think we don't need an new option, just remove vue loader from default loaders, and anyone who need it can add it optionally.

@danielroe
Copy link
Member

totally removing the vue loader would ultimately be ideal, but I'm trying to think of a interim solution that wouldn't require a major mkdist release.

@Teages
Copy link
Contributor Author

Teages commented Apr 4, 2025

@danielroe seems the package didn't release with types. I pushed a commit to fix it, can you release it on npm?

@zernonia
Copy link

zernonia commented Apr 4, 2025

@cyco130 @Dunqing has developed detype, which further patched (detypes) for shadcn-vue Vue SFC Typescript to Vue SFC Javascript transformation. Perhaps it might be similar to https://github.com/nuxt-contrib/vue-sfc-transformer?

@Teages
Copy link
Contributor Author

Teages commented Apr 4, 2025

@cyco130 @Dunqing has developed detype, which further patched (detypes) for shadcn-vue Vue SFC Typescript to Vue SFC Javascript transformation. Perhaps it might be similar to https://github.com/nuxt-contrib/vue-sfc-transformer?

@zernoia they were different, see #14

oops it they do the same work

After looking at their code, I think we will keep this PR. They have don't support for defeinModel yet and it would be more complex to do it in their current way.

@Teages
Copy link
Contributor Author

Teages commented Apr 4, 2025

totally removing the vue loader would ultimately be ideal, but I'm trying to think of a interim solution that wouldn't require a major mkdist release.

I think we can't avoid to release a major release if we have to considered this PR as a breaking change.

The issue #289 (comment) actually quite serious because we can't access any variables exposed from <script setup>, it prevent the most vue users from upgrading to mkdist v2.

@danielroe
Copy link
Member

Maybe we could revert back to the old one: skip transform when the sfc include <script setup>?

For context, @pi0 doesn't want to add the extra weight of the vue dependencies for the uses of mkdist that don't require vue.

I also think we might need to export the types for mkdist loaders + use them to create a mkdist loader in vue-sfc-transformer for easier use in other projects with mkdist.

@danielroe
Copy link
Member

@Teages I've now moved the vue loader from this PR into vue-sfc-transformer too, so we can develop it there in parallel...

@pi0 - what would you like to do? here are some options:

  • add vue-sfc-transformer as an optional peerDep, and require users to install it if they want to transform vue files... while it's technically a breaking change, it would be a simple migration, and we could even prompt users to do so
  • revert to previous vue loader from several iterations back + along with a flag to disable it, so downstream people could add their own vue loader (e.g. nuxt/module-builder)
  • release a major version and drop vue support

@danielroe
Copy link
Member

@Teages okay - we're good to go with adding vue-sfc-transformer as an optional peerDep + using it if it is present (otherwise staying with current implementation)

in next major we'll drop the current implementation

@Teages
Copy link
Contributor Author

Teages commented Apr 4, 2025

@danielroe I will make a minimal transform as fallback, can you release vue-sfc-transformer again? It doesn't export the type of mkdist loader

@danielroe
Copy link
Member

@Teages I'm pretty sure nuxt-contrib/vue-sfc-transformer@eb7e26d doesn't solve the issue.

I sent you a DM on Discord - maybe we could discuss there.

@danielroe danielroe changed the title feat(vue): use minimal transform for script setup feat(vue): use vue-sfc-transformer if installed Apr 4, 2025
@danielroe danielroe merged commit 6ee2537 into unjs:main Apr 4, 2025
2 checks passed
@Teages Teages deleted the feat/vue-ts-transpiler branch April 4, 2025 17:37
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.

vue template is not transpiled

3 participants