-
Notifications
You must be signed in to change notification settings - Fork 916
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(plugin-vue): enhancements #984
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/cxwhtksea [update for 0acf94e canceled] |
This makes sense to me, and seems to be a really valuable feature. would love to see this work finished and merged! |
c9f1f26
to
a195711
Compare
a195711
to
30d3050
Compare
30d3050
to
1861a54
Compare
1861a54
to
92c66ae
Compare
92c66ae
to
3824090
Compare
3824090
to
88eefad
Compare
@FredKSchott , finished. Need U review. |
Just confirming: are these limitations new, or do they currently exist in the plugin? I'm fine to merge as long as it is not a new regression that could knowingly break existing users. Otherwise, we can wait until a user reports running into this limitation before resolving it.
@gr2m is working on improving our tests for plugins, I'll let him answer/review this aspect of the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a couple of comments, and then would love @gr2m to review tests before merging.
These limitations are not exist in the plugin, because current plugin do not support jsx/tsx feature. So the limitations come with our new feature that will not break existing users. We can wait until a user reports running into this limitation before resolving it. (Agree with it)
Thank you for tell me the License problem, I understand with it. https://github.com/pikapkg/snowpack/pull/984/files#diff-52e9f81ed066f12a83aa4401fbbdddf7R1
according to : evanw/esbuild#366 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the thorough tests as well!
/cc @gr2m to take another look before merging, since he brought up the original testing questions. (Also, I'm +1 for bringing this plugin testing style to the rest of our plugins, since it seems like Jest just automatically runs these without any new config). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great! I agree that it would be fantastic to do similar tests for other plugins. I'll give it a try myself, and then start a follow up discussion so others can contribute
Would you prefer to add |
I prefer the latter. Because current app-template project always deps on an released plugin. https://github.com/pikapkg/snowpack/blob/master/create-snowpack-app/app-template-vue/package.json#L20 |
Changes
tsconfig
app-template-vue-typescript
1. Full support script lang ts in vue file
Use esbuild compile script content ts code to js code. ✌️ As
vite
, we can't support type check in compile time.https://github.com/pikapkg/snowpack/pull/984/files#diff-460291335ddc08075bf891e8e0c5f93fR64
2. Portion support script lang ts in vue file and tsx, jsx file
Use esbuild
jsxFactory
compile script tsx, jsx content and tsx, jsx files.https://github.com/pikapkg/snowpack/pull/984/files#diff-460291335ddc08075bf891e8e0c5f93fR63
If need to use tsx and jsx files, need the config
plugins/plugin-vue/plugin-tsx-jsx.js
.https://github.com/pikapkg/snowpack/pull/984/files#diff-460291335ddc08075bf891e8e0c5f93fR63
Limitation
May be have some limitation compare to: https://github.com/vuejs/jsx-next/tree/dev/packages/babel-plugin-jsx, such as un support vue directive
v-for
,v-if
,v-model
and so on.But we use the same way to resolve tsx, jsx code as
vite
, sovite
have the same limitation.https://github.com/vitejs/vite/blob/master/src/node/esbuildService.ts#L77
Should we need to support option to use babel compile and
babel-plugin-jsx
? Need U suggestion.3. Fix vue css hmr bug
For support style hmr update in vue file. The
wrapResponse
must be in frontend ofresolveResponseImports
https://github.com/pikapkg/snowpack/pull/984/files#diff-7101fc032cd14c955a7f47f91a7f7245R639
Project template
app-template-vue-typescript
New PR after this feature released.
Testing
Added:
plugins/plugin-vue/test
.How to manage the plugin test cases ? Need U suggestion.
Testing Demo Repo
Please try on: https://github.com/Akimyou/snowpack/tree/feature/plugin-vue-enhance-demo/create-snowpack-app/app-template-vue-typescript