feat(v2): Allow customization of js loader, replace babel by esbuild in Docusaurus website - #4766
Conversation
|
[V2] Built with commit 30d2b3b |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4766--docusaurus-2.netlify.app/ |
slorber
left a comment
There was a problem hiding this comment.
Thanks, Looks like a nice thing we could merge without too much risk!
Just not 100% fan of the current API
| } | ||
| )[]; | ||
| titleDelimiter?: string; | ||
| getCustomJSLoader?: (isServer: boolean) => RuleSetRule; |
There was a problem hiding this comment.
The directly I'd like to take is to group/namespace related Docusaurus config.
Also think we should accept simple values like "babel" | "esbuild" | "sucrase" later, and the name of the option should allow that.
What do you think of:
module.exports = {
webpack: {
jsLoader: "esbuild",
},
};It should still be possible to provide a jsLoader function, but we'll have the possibility to introduce jsLoader "presets" later
There was a problem hiding this comment.
This looks good to me. I think I will first allow the "babel" value in this PR and add in other options later.
When we are adding presets, not sure how opinionated we should be about the preset. It's probably not a good idea to directly include esbuild-loader as a dependency, since then users not using esbuild will also need to pay the cost of installing esbuild (which has a postinstall script to download a binary for the platform that takes several seconds). There is also some stability and upgrade concerns. For example, esbuild does releases very often and much faster than docusaurus. esbuild-loader has an option to provide their own esbuild package, not sure whether this is something we want to do.
There was a problem hiding this comment.
Agree, for now it seems simpler to just allow providing the custom esbuild loader and only let power-users opt-in for this until it is more stable and we know better the esbuild problems that we can encounter.
Eventually, we could later make the esbuild deps optional and require it in a try/catch, throwing/asking the user to provide his own esbuild package.
As we still use webpack, the benefits are not so huge for smaller sites.
I think if we add such API we should dogfood on docusaurus site itself to be sure using esbuild is possible in practice.
My first attempt did not work because it seems we really have to replace Terser (as you did #4532) or it fails with an error (not sure why)
If we merge this PR API, it wouldn't enable the user to replace the minimizer easily so users wouldn't really be able to adopt esbuild. I think we should make sure using esbuild is possible in practice by using it ourselves.
There was a problem hiding this comment.
Was also wondering, how do we handle polyfills when not using babel?
It is not 100% clear to me as we remove @babel/preset-env
Wondering why the bundle size decreased here (#4532 (comment)), we'd rather make sure using esbuild does not reduce browser support for example
There was a problem hiding this comment.
All this looks simple (ie just replacing one loader by another) but I suspect it hides much more complexity in practice 😅
There was a problem hiding this comment.
@slorber I made it work without using the minify plugin. I pushed back to #4532. The key is to target node12 when doing SSR:
return {
loader: require.resolve('esbuild-loader'),
options: {
loader: 'tsx',
format: isServer ? 'cjs' : undefined,
target: isServer ? 'node12' : 'es2017',
},
}Related issue in esbuild: evanw/esbuild#1084
There was a problem hiding this comment.
Also note on browser support and polyfill.
In the first iteration of the esbuild exploration, I mentioned that we can target es2017 since docusaurus doesn't plan to add ie support.
Polyfill is tricky since esbuild doesn't come with polyfill by default. We might have to add them by ourselves. However, from my experience it doesn't seem to an issue if you are using latest browsers. That being said, maybe babel needs to be kept as an option forever for people who really need polyfill.
There was a problem hiding this comment.
yes it seems you are right, replacing the minimizer does not look like required. As it probably has less benefits than replacing the loader, it may not be needed to provide an API to replace it right now.
I added the support for Docusaurus site in this PR, seems to build successfully
There was a problem hiding this comment.
The deploy preview looks fine on modern browsers I could test on.
We can clearly see some things looking like polyfills/helpers have been removed by comparing some JS outputs:
https://docusaurus.io/assets/js/main.08f09d32.js
https://deploy-preview-4766--docusaurus-2.netlify.app/assets/js/main.a47930a3.js
Not 100% sure of the impact.
Note I tried to use new ES methods like flatMap or fromEntries and in both cases I don't see being polyfilled, so not sure polyfills were setup anyway 😅
Looks also related to this issue: evanw/esbuild#1230
Will likely merge this soon but keep it undocumented. If it's proven successful on our website we can add some doc
|
[V1] Built with commit 30d2b3b |
This reverts commit e656c35
|
Was able to get a free opensource license of BrowserStack, tested on a few older browsers and that looks safe enough for me to deploy! |
…in Docusaurus website (facebook#4766) * feat(v2): Allow customization of js loader * Change API * use esbuild for Docusaurus website * Enable isolatedModules: true * Revert "Enable isolatedModules: true" This reverts commit e656c35 Co-authored-by: slorber <lorber.sebastien@gmail.com>
Motivation
#4765 mentions that we should allow the user to choose a different js loader. As a first step, I decide to allow the user to choose an arbitrary js loader using a function inside the docusaurus config. From there, we can later add some opinionated choices later.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Related PRs
N/A