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.

Sign up for GitHub

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

feat(v2): Allow customization of js loader, replace babel by esbuild in Docusaurus website #4766

Merged
merged 7 commits into from
May 14, 2021
Merged

feat(v2): Allow customization of js loader, replace babel by esbuild in Docusaurus website #4766

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f2f9afd
feat(v2): Allow customization of js loader
SamChou19815 May 11, 2021
b9cd75b
Merge branch 'master' into custom-js-loader
SamChou19815 May 12, 2021
11e9aad
Change API
SamChou19815 May 12, 2021
848621e
use esbuild for Docusaurus website
slorber May 13, 2021
e656c35
Enable isolatedModules: true
slorber May 13, 2021
6fb56c9
Revert "Enable isolatedModules: true"
slorber May 14, 2021
30d2b3b
Merge branch 'master' into custom-js-loader
slorber May 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/docusaurus-types/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export interface DocusaurusConfig {
}
)[];
titleDelimiter?: string;
getCustomJSLoader?: (isServer: boolean) => RuleSetRule;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

image

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@slorber slorber May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

All this looks simple (ie just replacing one loader by another) but I suspect it hides much more complexity in practice 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

image

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

}

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/docusaurus/src/commands/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,14 @@ async function buildLocale({
configureWebpack.bind(plugin), // The plugin lifecycle may reference `this`.
clientConfig,
false,
props.siteConfig.getCustomJSLoader,
);

serverConfig = applyConfigureWebpack(
configureWebpack.bind(plugin), // The plugin lifecycle may reference `this`.
serverConfig,
true,
props.siteConfig.getCustomJSLoader,
);
}
});
Expand Down
1 change: 1 addition & 0 deletions packages/docusaurus/src/commands/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ export default async function start(
configureWebpack.bind(plugin), // The plugin lifecycle may reference `this`.
config,
false,
props.siteConfig.getCustomJSLoader,
);
}
});
Expand Down
1 change: 1 addition & 0 deletions packages/docusaurus/src/server/configValidation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ const ConfigSchema = Joi.object({
tagline: Joi.string().allow(''),
titleDelimiter: Joi.string().default('|'),
noIndex: Joi.bool().default(false),
getCustomJSLoader: Joi.function(),
});

// TODO move to @docusaurus/utils-validation
Expand Down
1 change: 1 addition & 0 deletions packages/docusaurus/src/webpack/__tests__/base.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ describe('base webpack config', () => {
const props = {
outDir: '',
siteDir: '',
siteConfig: {},
baseUrl: '',
generatedFilesDir: '',
routesPaths: '',
Expand Down
28 changes: 27 additions & 1 deletion packages/docusaurus/src/webpack/__tests__/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
* LICENSE file in the root directory of this source tree.
*/

import {validate, Configuration} from 'webpack';
import {validate, Configuration, RuleSetRule} from 'webpack';
import path from 'path';

import {
getCustomizableJSLoader,
applyConfigureWebpack,
applyConfigurePostCss,
getFileLoaderUtils,
Expand All @@ -18,6 +19,31 @@ import {
ConfigureWebpackFnMergeStrategy,
} from '@docusaurus/types';

describe('customize JS loader', () => {
test('getCustomizableJSLoader defaults to babel loader', () => {
expect(getCustomizableJSLoader()({isServer: true}).loader).toBe(
require.resolve('babel-loader'),
);
expect(getCustomizableJSLoader()({isServer: false}).loader).toBe(
require.resolve('babel-loader'),
);
});

test('getCustomizableJSLoader allows customization', () => {
const customJSLoader = (isServer: boolean): RuleSetRule => ({
loader: 'my-fast-js-loader',
options: String(isServer),
});

expect(getCustomizableJSLoader(customJSLoader)({isServer: true})).toEqual(
customJSLoader(true),
);
expect(getCustomizableJSLoader(customJSLoader)({isServer: false})).toEqual(
customJSLoader(false),
);
});
});

describe('extending generated webpack config', () => {
test('direct mutation on generated webpack config object', async () => {
// fake generated webpack config
Expand Down
5 changes: 3 additions & 2 deletions packages/docusaurus/src/webpack/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import path from 'path';
import {Configuration} from 'webpack';
import {Props} from '@docusaurus/types';
import {
getJSLoader,
getCustomizableJSLoader,
getStyleLoaders,
getFileLoaderUtils,
getCustomBabelConfigFilePath,
Expand Down Expand Up @@ -64,6 +64,7 @@ export function createBaseConfig(
const {
outDir,
siteDir,
siteConfig,
baseUrl,
generatedFilesDir,
routesPaths,
Expand Down Expand Up @@ -196,7 +197,7 @@ export function createBaseConfig(
test: /\.(j|t)sx?$/,
exclude: excludeJS,
use: [
getJSLoader({
getCustomizableJSLoader(siteConfig.getCustomJSLoader)({
isServer,
babelOptions: getCustomBabelConfigFilePath(siteDir),
}),
Expand Down
20 changes: 17 additions & 3 deletions packages/docusaurus/src/webpack/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export function getBabelOptions({

// Name is generic on purpose
// we want to support multiple js loader implementations (babel + esbuild)
export function getJSLoader({
function getDefaultBabelLoader({
isServer,
babelOptions,
}: {
Expand All @@ -150,6 +150,19 @@ export function getJSLoader({
};
}

export const getCustomizableJSLoader = (
getCustomJSLoader?: (isServer: boolean) => RuleSetRule,
) => ({
isServer,
babelOptions,
}: {
isServer: boolean;
babelOptions?: TransformOptions | string;
}): RuleSetRule =>
getCustomJSLoader
? getCustomJSLoader(isServer)
: getDefaultBabelLoader({isServer, babelOptions});

// TODO remove this before end of 2021?
const warnBabelLoaderOnce = memoize(function () {
console.warn(
Expand All @@ -163,7 +176,7 @@ const getBabelLoaderDeprecated = function getBabelLoaderDeprecated(
babelOptions?: TransformOptions | string,
) {
warnBabelLoaderOnce();
return getJSLoader({isServer, babelOptions});
return getDefaultBabelLoader({isServer, babelOptions});
};

// TODO remove this before end of 2021 ?
Expand All @@ -190,11 +203,12 @@ export function applyConfigureWebpack(
configureWebpack: ConfigureWebpackFn,
config: Configuration,
isServer: boolean,
getCustomJSLoader?: (isServer: boolean) => RuleSetRule,
): Configuration {
// Export some utility functions
const utils: ConfigureWebpackUtils = {
getStyleLoaders,
getJSLoader,
getJSLoader: getCustomizableJSLoader(getCustomJSLoader),
getBabelLoader: getBabelLoaderDeprecated,
getCacheLoader: getCacheLoaderDeprecated,
};
Expand Down