-
-
Notifications
You must be signed in to change notification settings - Fork 278
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
Replace webpack with esbuild. #1684
base: main
Are you sure you want to change the base?
Conversation
Hello, I think this is an error on your side or that your changes are incomplete as ionide-vscode-fsharp/build/Program.fs Lines 279 to 280 in 69889cc
or even the workflow:
|
Hey Maxime, good spot, I indeed did not indent to remove the yarn lock but commit the modified version. |
@nojaf TBH I think I originally missread the PR 😅 When I saw |
Aha, I see, yeah swapping this is good for the node_modules footprint. |
Hey hope you don't mind this, I re-tried to remove webpack from the toolchain in #1722 I don't think we need esbuild (preprocess javascript) at all so I went with rollup (just bundles and tree-shakes) in terms of bundling rollup is more flexible (but still simpler than webpack by a mile or two) and could allow us to work in some more complex webviews in the future Feel free to let me know if that works or should we stick with esbuild |
Hello, I do kinda mind actually. Esbuild is literally listed as the first item on the docs https://code.visualstudio.com/api/working-with-extensions/bundling-extension. You don't go into detail about why rollup is the better fit, so why should we deviate from what is recommended?
That is an assumption, you don't know what old vscode version is going to be used (older electron, older V8 runtime). Nor are you considering cloud IDE scenario's where older browsers might be a thing due to company policies. |
Of course, let me formulate Fable emits ES2015 compatible code, which means every browser from 2016~2017 already supports it, thankfully Internet Explorer is already EoL, in the case of electron it is tightly coupled to the chromium engine, so it supports ES2015 from around the same time, in the case of node.js the ES2015 support was introduced around version 8, and full support was fully added in node 10 AFAIK both this versions have gone EoL as well, so I can feel quite safe saying that we won't face any particular issue with browser vendors or electron version, even if they date back a couple of years old which even then it would be very bad for security purposes of any company if that was the case
Esbuild is not the only one mentioned in the docs, in the brief mention on all of them rollup is actually the first mentioned but I digress
Rollup is a better fit from a configuration standpoint, rollup can provide multiple bundles if necessary (a simple example can be seen here) which could be really useful for vscode webviews,I made some discoveries in that area in #1721, it has a broad set of plugins and a healthy ecosystem and it is often used as the bundler for popular tools like vite, which uses esbuild for the transpilation but leverages rollup even for it's plugin API
If Ionide authors were not using fable for the codebase I would favor esbuild given that one may want to use decorators today or private javascript properties or some form of newer syntax but thankfully that's not the case I do believe we don't need any kind of transpiling to older javascript and only the bundling flexibility is needed in this project
|
You could have multiple invocations of
Replacing the If we are certain that the ES20XX code by Fable runs everywhere I'd even be in favour of not bundling at all. It seemed the safest thing to do, but if that is a thing of the past then there is no need. |
@nojaf Bundling still makes sense nowdays because:
My 2 cents, is that if esbuild does the job let's use it. If in the feature we need specific features that esbuild doesn't support it is always possible to revisit the build process. |
The libraries surrounding rollup are fairly more stable and the packages needed are actually updated by the core rollup team, we don't need to depend on packages outside core, in any case the configuration for rollup is far simpler than that of webpack, we don't need to meddle with loaders and weird resolution rules, since rollup is meant to be used with modern javascript.
I agree there it's a simpler alternative and in conjunction with pnpm disk footprint can lower down quite a lot (improving CI times as well)
I actually asked that question #1672 (comment) whether we wanted bundling or not I don't think we do, but if we need to bundle anyway because the docs says it is a good practice I personally would trust more rollup than esbuild in that aspect but that's just personal opinion |
Well... I think we do need bundling now that I remember vscode extensions don't seem support ESModules (although node does) we need to have a commonjs bundle/output I stumbled upon this when I was moving to rollup on my PR other point I think we're both missing was that Chet mentioned we should include the F# sourcemaps for better debugging |
Fixes #1672.
Followed the instructions at https://code.visualstudio.com/api/working-with-extensions/bundling-extension#using-esbuild.
The watch target is no longer building, I was able to run the launch target after Fable compiled the files.
What a world we live in.