-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
feat(v2): Allow configuring babel via babel.config.js #2903
Conversation
Deploy preview for docusaurus-2 ready! Built with commit aba33b4 |
Thanks Sam, will leave to @slorber to review! |
website/docs/docusaurus.config.js.md
Outdated
presets: [ | ||
'@babel/preset-flow', | ||
// The following lines are necessary, | ||
// unless you want to configure Babel from scratch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to somehow add custom Babel plugins without adding these lines? I feel this is a little verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a package babel-merge that seems to do the job. However, it is not maintained by babel so I'm not sure whether we should use it, instead of relying on babel's preset merging mechanism that is guaranteed to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on usages/issues that seems pretty reliable, but I think we should build our own preset so that we don't need this, as discussed here: #2903 (comment)
TLRD: I think we should build our own babel presetYour current solution works but I'm not totally fan of the API.
Actually, I was surprised by what you said here, as I've seen many isServer or isProduction things in many fwk codebases (Next, Gatsby, Expo, RN/Metro...) codebase. I think the approach used by Next is suitable for our usecase. It uses the "caller" concept of Babel to pass flags to the preset, so that the preset can have a dynamic configuration: https://babeljs.io/docs/en/options#caller Here's what I would try: Create a Docusaurus presetUse the caller api to know if the preset is used in server/client mode, dev/prod mode, and create the appropriate babel config.
Next does this: https://github.com/vercel/next.js/blob/31b3e46b8f08f3ae8c16d95f4379f3129324a9be/packages/next/build/babel/preset.ts#L66 Alternative, the babel loader already injects a caller.target === "node" / "web" so that you know if isServer is true in the preset Use the preset in the Babel webpack loaderThe loader option should tell the Docusaurus preset in which mode it is
Next does this: Add a .babelrc with the Docusaurus preset:
This is the public api that most users would expect. Next does this: I think we could by default add a For retrocompatibility we could default to a config with just the preset. I've seen this applied in Metro for RN here: https://github.com/facebook/metro/blob/master/packages/metro-react-native-babel-transformer/src/index.js#L91 What do you think @SamChou19815 @yangshun @lex111 ? |
website/docs/docusaurus.config.js.md
Outdated
presets: [ | ||
'@babel/preset-flow', | ||
// The following lines are necessary, | ||
// unless you want to configure Babel from scratch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on usages/issues that seems pretty reliable, but I think we should build our own preset so that we don't need this, as discussed here: #2903 (comment)
I've also looked at other solutions: GatsbyGatsby has a dedicated lifecycle api to update the webpack/babel config directly at the plugin level. Probably more advanced than we want here. The Gatsby setup is quite complicated, with multiple babel stages (one for example is dedicated to extract the static queries from the code). It does not seem to use the caller api but rather build the final config (with its own babel config merge logic), and then call the different babel stages one by one, with this config. https://github.com/gatsbyjs/gatsby/blob/master/packages/babel-preset-gatsby/src/index.js Also, in my experience trying to use a .babelrc in it, it didn't work great ^^ Not sure we should be inspired by Gatsby for these reasons. ExpoExpo uses the caller api so that the preset can know for which platform it builds (ios/android/web) and adapt the preset accordingly https://github.com/expo/expo/blob/master/packages/babel-preset-expo/index.js#L8 |
@slorber updated my implementation! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Tested locally and it worked fine.
Just remove the configureBabel
Other changes are more subjective, as the feature is working correctly
Do you think it could be useful to publish the preset as a separate package? I don't think it would be useful, but others are doing this so... don't know the reasons
BABEL_CONFIG_FILE_NAME, | ||
)); | ||
} catch { | ||
customBabelConfiguration = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works but not fan of using exception flow, I'd rather use fs.existsSync(path)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not read/require that file, we could just check if it exists and if it does, put configFile: true
in babel so that it picks it automatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
babelrc: false, | ||
configFile: false, | ||
caller: {name: isServer ? 'server' : 'client'}, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the config file exist, what about configFile: true (or providing its path explicitly?)
I don't like so much the idea to have a custom assign/merge here (but it probably does not matter much anyway)
```js title="babel.config.js" | ||
module.exports = { | ||
presets: [require.resolve('@docusaurus/core/lib/babel/preset')], | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require.resolve() is for yarn2 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
It's definitely easier to keep it within |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All seems good, but I'd rather simplify getBabelLoader signature before merge
options: Object.assign( | ||
export function getBabelLoader( | ||
isServer: boolean, | ||
babelOptions?: TransformOptions | string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not worth to keep the babelOptions object here, as it's an internal method and it won't ever be called with an object.
I'd rather have:
export function getBabelLoader({isServer, configFile}: {isServer: boolean, configFile?: string}) {
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not completely internal. It has been exposed here: https://v2.docusaurus.io/docs/lifecycle-apis#configurewebpackconfig-isserver-utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah :) that's fine then, I guess we need to be retrocompatible
options: Object.assign( | ||
export function getBabelLoader( | ||
isServer: boolean, | ||
babelOptions?: TransformOptions | string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah :) that's fine then, I guess we need to be retrocompatible
@slorber feel free to merge it, you're also a maintainer and you're familiar with the code already :) |
I'm afraid to forget squashing now ;) |
Motivation
Close #2772. Allow user to configure babel using
babel.config.js
. For backward compatibility, lack of this file will automatically fallback to default config.Using advice from @slorber, it is implemented in a way without exposing
isServer
internals to the end user.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
No custom babel config
The current repo doesn't use custom babel config, and the preview site can still build.
With custom babel config
website/babel.config.js
andyarn start
, website can still load.website/src/pages/index.js
yarn start
, and observe that the site failed to load with syntax errors.yarn workspace docusaurus-2-website add @babel/preset-flow
website/babel.config.js
to:yarn start
, and observe that the site can correctly load.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.)