improve support for svelte npm packages #1908
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/62czmr771 |
FredKSchott
force-pushed
the
svelte-plugin-esinstall-fix
branch
from
a562520 to
df9bece
Compare
FredKSchott
force-pushed
the
svelte-plugin-esinstall-fix
branch
from
df9bece to
beda213
Compare
FredKSchott
force-pushed
the
svelte-plugin-esinstall-fix
branch
from
b1aa986 to
d1e092a
Compare
FredKSchott
force-pushed
the
svelte-plugin-esinstall-fix
branch
from
d1e092a to
26653a7
Compare
| @@ -45,7 +45,7 @@ export {printStats} from './stats'; | |||
|
|
|||
| type DependencyLoc = | |||
| | { | |||
| type: 'JS'; | |||
| type: 'BUNDLE'; | |||
| @@ -359,7 +359,7 @@ export async function install( | |||
| cwd, | |||
| packageLookupFields, | |||
| }); | |||
| if (resolvedResult.type === 'JS') { | |||
| if (resolvedResult.type === 'BUNDLE') { | |||
There was a problem hiding this comment.
Do we not need Edit: oh nvm; it was just renamedJS handling anymore?
There was a problem hiding this comment.
yea, renamed to something more inclusive of files like .svelte. Basically: should this be bundled by Rollup, or should this be copied over 1:1 as an asset and not passed through rollup.
| return 'BUNDLE'; | ||
| } | ||
| // All other files should be treated as assets (copied over directly). | ||
| return 'ASSET'; |
There was a problem hiding this comment.
So do we not need to handle JSX/TSX because of built-in support from esbuild? Curious if that’s really a different strategy than this (though JSX not carrying CSS seems different enough on its own)
There was a problem hiding this comment.
I don't think we have a real use-case for a user publishing JSX/TSX in a package. Remember, this is just a way to properly categorize npm package entrypoints.
There was a problem hiding this comment.
I don't think we have a real use-case for a user publishing JSX/TSX in a package.
I can say myself that I’ve tried, and been punished by webpack 😅 . I think just as a consequence of the ecosystem I’d guess most popular React packages have been forced to prebundle JSX and this statement is correct
There was a problem hiding this comment.
yea that's fair enough, and we want that error to be reported instead of the JSX file copied over 1:1 as if it were an image.
+1 for a PR to add JS, JSX, TS, and TSX here to this function AND add a curated JSX found, add this plugin to support error message like we have for Vue and Svelte.
| ).rejects.toThrowError(`Install failed.`); | ||
| // TODO: | ||
| // Assert the reason: Failed to load ../../../node_modules/simple-svelte-autocomplete/src/SimpleAutocomplete.svelte | ||
| // Try installing rollup-plugin-svelte and adding it to Snowpack (https://www.snowpack.dev/#custom-rollup-plugins) |
There was a problem hiding this comment.
👍🏻
Should we add the reason in this PR? Or does that require a bigger change?
There was a problem hiding this comment.
I could have made this more clear: that is currently what running esinstall will log, but it does so in a separate console.error call instead of the actual throw error. This is that issue i'd mentioned at standup today.
I think my vote would be to add these currently-console-error'd messages as a error.reason property, but we can tackle in a future PR
Changes
.svelte&.vuefiles (instead of treating them as assets).*.sveltefiles.Testing
Docs