search

jspm / jspm.org

jspm.org website
https://jspm.org
13 stars 17 forks source link

Add `vitejs workflow using jspm #16

Closed JayaKrishnaNamburu closed 2 years ago

JayaKrishnaNamburu commented 2 years ago

Still a few more grammatical corrections and wording suggetsions.

I'm still not sure I follow why we ever need to set polyfillDynamicImport: false? Does Vite polyfill dynamic import itself? If so does the Vite plugin system allow plugins to alter its own input options? It just seems to me like this should always be the default for the plugin and users shouldn't even need to set it? When would you not want it?

Sorry, English isn't my strongest thing 😅. It's my third language 😁. Regarding polyfilling flag. I will check it now. Why we are doing it, but it's a vite flag for sure. It's a config and plugins are not allowed to change it. It's just that users need to assign it in the config.

JayaKrishnaNamburu commented 2 years ago

Interesting, looks like polyfillDynamicImport it not a valid option anymore in vitej. https://vitejs.dev/config/build-options.html And regarding other polyfillModulePreload for now users can keep it as true or false. It's not applying. But the new @jspm/generator can take in html as input. So, users can configure that option using the vite-plugin-jspm itself. And the generator can take care of it. Even vite is doing the same and referring to the jspm article. https://vitejs.dev/config/build-options.html#build-polyfillmodulepreload https://guybedford.com/es-module-preloading-integrity#modulepreload-polyfill

In the usage, the comment says https://github.com/jspm/vite-plugin-jspm#usage

// we need to disable vite's default polyfilling, because es-module-shims enables it instead

That gives me the doubt, if vite is polyfilling it. Does es-module-shims is able to detect it in runtime and not add it's own polyfill. Since the detection happens on browser level. And shims is able to detect polyfills done by the users and enable/disable according ?

Is so, we don't need to ask users to disable this flag.

guybedford commented 2 years ago

What I mean is the Vite plugin system has its own config hook - https://github.com/jspm/vite-plugin-jspm/blob/main/plugin/src/index.ts#L52.

So we should just be able to do config.build.polyfillModulePreload = false in the Vite plugin itself in that hook and then there's no need for a user configuration here.

I'm happy to work through the grammar, but let's iron out these technical details first.

I know I ask for a lot of feedback, sorry if it's a pain, and I understand if that puts you off creating PRs!

JayaKrishnaNamburu commented 2 years ago

No it's ok, no problem at all. I am just trying to understand if we need to disable the vite polyfills for module preload. Or is shims able to detect that. Because if it can, we don't need to enforce or change anything on the configs w.r.t. to build. I tried by leaving the flag empty and doesn't seem to have any effect on the build. It is not creating preload links in the html either. I will check this behaviour today 👍

JayaKrishnaNamburu commented 2 years ago

Made the changes and, i looked into the config thing. Yes, we can change the config from plugin. No need for users to do it manually. Adding it to the plugin in https://github.com/jspm/vite-plugin-jspm/pull/3

guybedford commented 2 years ago

Nice, thanks for working through all these! I added some further changes for merge, let me know if that all seems ok to you.