Skip to content
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

[DO-NOT-MERGE] RFC: use esbuild to significantly speedup building #4487

Closed
wants to merge 1 commit into from
Closed

[DO-NOT-MERGE] RFC: use esbuild to significantly speedup building #4487

wants to merge 1 commit into from

Conversation

SamChou19815
Copy link
Contributor

Motivation

This PR tries to use esbuild to compile all the JS code (including all the mdx-react produced js code). Since esbuild is written in Go with performance in mind, it results in a much faster build. Locally, the server build takes 1min for each locale without this change, and only 40s with this change.

However, I do recognize that this PR is not ready for merge right now. I understand that many websites might depend on using specific babel config, and probably esbuild's min target es2015 is not good enough. The maintainers might also want to put this behind a flag (either in docusaurus.config.js or a env-var). I only created this PR as a proof of concept how it can be done and to show how fast it can speed up build in CI, and wait on maintainers' opinion on whether and how we want to move forward with this. Therefore, I haven't setup any gating yet and simply replaces babel config and minimize plugin with esbuild stuff.

I'm open to keep this behind a flag or only as a plugin that imperatively patch the webpack config, like the one I used for my personal website.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

CI

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@SamChou19815 SamChou19815 marked this pull request as draft March 21, 2021 16:34
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 21, 2021
@netlify
Copy link

netlify bot commented Mar 21, 2021

@netlify
Copy link

netlify bot commented Mar 21, 2021

@github-actions
Copy link

github-actions bot commented Mar 21, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 79
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4487--docusaurus-2.netlify.app/

@SamChou19815
Copy link
Contributor Author

SamChou19815 commented Mar 21, 2021

This PR improves the build time of server from 2.90m to 1.92m in this PR, that's a 30% reduction. (See the build size log)

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, yes, we definitely need it! 👍 I thought about esbuild using too, especially when I looked at benchmark results https://github.com/privatenumber/minification-benchmarks#-results
Very impressive, we have to give it a chance. Manage it via env var as an experimental feature is good idea.

@armano2
Copy link
Contributor

armano2 commented Mar 22, 2021

This PR improves the build time of server from 2.92m in previous PR to 1.61m in this PR

bdw, both links point to same buld

@SamChou19815
Copy link
Contributor Author

@armano2 whoops, updated it

@armano2
Copy link
Contributor

armano2 commented Mar 22, 2021

Size Change: +3.81 MB (+663%) 🆘

Total Size: 4.39 MB

Filename Size Change
website/build/assets/css/styles.********.css 87.1 kB -48 B (0%)
website/build/assets/js/main.********.js 4.22 MB +3.82 MB (+951%) 🆘
website/build/blog/2017/12/14/introducing-docusaurus/index.html 60.3 kB -498 B (-1%)
website/build/index.html 25.5 kB -242 B (-1%)
ℹ️ View Unchanged
Filename Size Change
website/build/docs/introduction/index.html 235 B 0 B

compressed-size-action

@SamChou19815
Copy link
Contributor Author

SamChou19815 commented Mar 22, 2021

Looks like the bloated the size is due to esbuild not removing license comments... Might need other hacks for that...

Nevermind, the problem is actually code-splitting is broken. Now it's fixed, and we even have some size reductions:

Filename Size Change
website/build/assets/js/main.********.js 397 kB -4.67 kB (-1%)
ℹ️ View Unchanged
Filename Size Change
website/build/assets/css/styles.********.css 87.2 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 60.8 kB 0 B
website/build/docs/introduction/index.html 235 B 0 B
website/build/index.html 25.7 kB 0 B

Turns out I need this change to overcome this issue in esbuild.

   return {
     loader: require.resolve('esbuild-loader'),
-    options: {loader: 'tsx', format: 'cjs', target: 'es2015'},
+    options: {
+      loader: 'tsx',
+      format: isServer ? 'cjs' : 'esm',
+      target: 'es2017',
+    },
   };
 }

},
}),
];
const minimizer = [new ESBuildMinifyPlugin({target: 'es2017'})];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched output to es2017, since v2 doesn't plan to support ie11, and all other major browsers have good support for es2017: https://caniuse.com/?search=es2017

Comment on lines +8 to +10
"build": "tsc && yarn build-theme-esm && yarn build-client-esm",
"build-theme-esm": "esbuild src/theme/hooks/useDocs.ts > lib/theme/hooks/useDocs.js",
"build-client-esm": "esbuild src/client/docsClientUtils.ts > lib/client/docsClientUtils.js",
Copy link
Contributor Author

@SamChou19815 SamChou19815 Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow esbuild doesn't like that the client files are not in esm, so I setup the script to compile the client files to esm.

Comment on lines -11 to +17
const {transform, features: bubleFeatures} = require('@philpl/buble');
import {transform, features as bubleFeatures} from '@philpl/buble';

// This file is designed to mimic what's written in
// https://github.com/kitten/buble/blob/mini/src/index.js, with custom transforms options,
// so that webpack can consume it correctly.
exports.features = bubleFeatures;

exports.transform = function customTransform(source, options) {
function customTransform(source, options) {
return transform(source, {...options, transforms: {asyncAwait: false}});
};
}

export {bubleFeatures as features, customTransform as transform};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to turn this into esm to make esbuild happy.

Comment on lines 145 to 158
export function getBabelLoader(
isServer: boolean,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
babelOptions?: TransformOptions | string,
): Loader {
return {
loader: require.resolve('babel-loader'),
options: getBabelOptions({isServer, babelOptions}),
loader: require.resolve('esbuild-loader'),
options: {
loader: 'tsx',
format: isServer ? 'cjs' : 'esm',
target: 'es2017',
},
};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to make this function be like

if (process.env.ESBUILD) {
  return {
    loader: require.resolve('esbuild-loader'),
    options: {
      loader: 'tsx',
      format: isServer ? 'cjs' : 'esm',
      target: 'es2017',
    },
  };
  return {
    loader: require.resolve('babel-loader'),
    options: getBabelOptions({isServer, babelOptions}),
  }
}

In this way all the existing code that uses the getBabelLoader can work without modification. This is somehow needed since this function is exposed here. However, this is bad since the function should then really be named as getJSLoader. Or we can create a new function called getJSLoader and deprecate the getBabelLoader function.

Do maintainers have any options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getJSLoader is also something I had in mind. Would be fine to deprecate getBabelLoader()

@SamChou19815 SamChou19815 marked this pull request as ready for review March 23, 2021 00:34
@SamChou19815
Copy link
Contributor Author

looks like the recent bump of esbuild breaks a lot of things...

@SamChou19815
Copy link
Contributor Author

Closing this now... looks like patching the webpack config doesn't quite work since there are more moving parts in the SSG part that I don't understand. I may revisit this when I figure out how things work.

@SamChou19815 SamChou19815 deleted the esbuild branch March 28, 2021 22:04
Comment on lines 145 to 158
export function getBabelLoader(
isServer: boolean,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
babelOptions?: TransformOptions | string,
): Loader {
return {
loader: require.resolve('babel-loader'),
options: getBabelOptions({isServer, babelOptions}),
loader: require.resolve('esbuild-loader'),
options: {
loader: 'tsx',
format: isServer ? 'cjs' : 'esm',
target: 'es2017',
},
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getJSLoader is also something I had in mind. Would be fine to deprecate getBabelLoader()

}

return minimizer;
return [new ESBuildMinifyPlugin({target: 'es2017', css: true})];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder how much different this minification is compared to the old one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier stat because I mess up the build shows that it reduces the output by 1%.

@slorber
Copy link
Collaborator

slorber commented Mar 29, 2021

Thanks @SamChou19815 , that's nice to see that integrating esbuild with our Webpack setup is not too complicated and to have some perf numbers.

On the short term the plan is to merge the Webpack 5 migration (#4089), add auto-generated sidebars and prepare a beta release.

After that, I would be happy to enable an experimental esbuild setup, and maybe we could even make it the default if it is successful and the site does not have any custom babel config (or if it only contains our preset?).

Something important to note: I'd like the i18n <Translate> component to be a "zero-runtime" API and have a Babel plugin inline the translations directly in the page markup instead of loading a global i18n bundle (not correctly code-splitted). We'll have to figure out if this is possible with esbuild, as afaik it may provide fewer capabilities compared to Babel?

@SamChou19815
Copy link
Contributor Author

Something important to note: I'd like the i18n <Translate> component to be a "zero-runtime" API and have a Babel plugin inline the translations directly in the page markup instead of loading a global i18n bundle (not correctly code-splitted). We'll have to figure out if this is possible with esbuild, as afaik it may provide fewer capabilities compared to Babel?

I think we can add a loader that deals with this before the esbuild loader?

@SamChou19815
Copy link
Contributor Author

Redoing it in #4532.

@slorber
Copy link
Collaborator

slorber commented Mar 29, 2021

I think we can add a loader that deals with this before the esbuild loader?

Yeah, we could try, but that would somehow defeat the purpose of using esbuild in the first place as it requires parsing twice the JS code? Not sure how expensive it would be

@SamChou19815
Copy link
Contributor Author

Yeah, we could try, but that would somehow defeat the purpose of using esbuild in the first place as it requires parsing twice the JS code? Not sure how expensive it would be

I think it's truly unavoidable to run JS code in this case, unless we also write the same transformation code in go as a esbuild go plugin, which I think it's too much to maintain. We can potentially avoid double parsing by detecting whether there is import on @docusaurus/Translate or <Translate...

@slorber
Copy link
Collaborator

slorber commented Mar 30, 2021

We don't have to implement this optim soon anyway, it's not a big deal for now, but would just like to keep the opportunity to solve this 18n code-splitting problem someday :) If esbuild integration is successful we could build a go plugin in 6 months or 1 year to optimize things for Docusaurus even more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants