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
Use new babel API's to manage babel options #6801
Conversation
|
||
module.exports = ({ stage, browserslist, resolve = require.resolve }) => | ||
babelLoader.customLoader(babel => { | ||
const presetEnv = babel.createConfigItem([ |
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'm not sure i'm doing this right
// Passed Babel's 'PartialConfig' object. | ||
config(partialConfig) { | ||
// TODO: we should run partialConfig through the gatsby babel API plugin | ||
// The problem there is this is sync and the API run is async :/ |
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.
this is an interesting problem. we have no way of doing sync node api calls...
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 guess we could just do them all ahead of time?
Deploy preview for gatsbygram ready! Built with commit 66de295 |
Deploy preview for using-postcss-sass failed. Built with commit 66de295 https://app.netlify.com/sites/using-postcss-sass/deploys/5b737b503813f03812ac0932 |
Deploy preview for using-styled-components failed. Built with commit ac1ea8f https://app.netlify.com/sites/using-styled-components/deploys/5b75d74c3813f00495ac0908 |
Deploy preview for image-processing failed. Built with commit ac1ea8f https://app.netlify.com/sites/image-processing/deploys/5b75d74b3813f00495ac0902 |
Deploy preview for using-drupal ready! Built with commit 66de295 |
Deploy preview for using-jss failed. Built with commit 6b27a5c https://app.netlify.com/sites/using-jss/deploys/5b7371f382d3f120786b4244 |
Deploy preview for using-contentful failed. Built with commit ac1ea8f https://app.netlify.com/sites/using-contentful/deploys/5b75d74c3813f00495ac090b |
Deploy preview for using-glamor failed. Built with commit ac1ea8f https://app.netlify.com/sites/using-glamor/deploys/5b75d74b3813f00495ac0905 |
Deploy preview for using-remark failed. Built with commit 6b27a5c https://app.netlify.com/sites/using-remark/deploys/5b7371f182d3f120786b422f |
i know bots! they all gonna fail |
Deploy preview for gatsbyjs failed. Built with commit bdca781 https://app.netlify.com/sites/gatsbyjs/deploys/5b733fe13813f0655cc338c2 |
working on this now btw |
So this seems to be working now. Basically what I arrived at is that we run the same API calls in @jquense & @loganfsmyth would love to hear your thoughts |
}) | ||
const babelrcState = store.getState().babelrc | ||
const babelState = JSON.stringify(babelrcState.stages, null, 4) | ||
fs.writeFile(directoryPath(`.cache/babelState.json`), babelState) |
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 this missing an await
?
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 :-D
@@ -28,6 +28,12 @@ module.exports = async ( | |||
) => { | |||
const directoryPath = withBasePath(directory) | |||
|
|||
await fs.writeFile( |
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 the stage able to be passed through to the loaders as an argument? That seems like it would be easier.
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.
No idea... I'll just set it on process.env
actually instead of this method.
return { | ||
loader: { | ||
cacheDirectory: true, | ||
babelrc: false, |
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 don't think you'd want to set this since you have partialConfig.hasFilesystemConfig()
explicitly checked.
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.
👍
const reduxPresets = [] | ||
pluginBabelConfig[stage].plugins.forEach(plugin => { | ||
reduxPlugins.push( | ||
babel.createConfigItem([require.resolve(plugin.name), plugin.options], { |
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.
Might be worth caching this somehow to avoid re-resolving for every single file that Gatsby compiles. Or does Node already cache that for require
?
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.
Yeah, node caches that so we should be fine.
Think this is ready for another review @jquense, @loganfsmyth, @pieh, @m-allanson. Main thing now left to do is add some tests + do some more manual testing. Manually tested scenarios:
|
Nice! I'm not familiar with babel-loader's |
www/package.json
Outdated
"gatsby-plugin-manifest": "next", | ||
"gatsby-plugin-netlify": "next", | ||
"gatsby-plugin-manifest": "^2.0.2-beta.5", | ||
"gatsby-plugin-netlify": "^2.0.0-beta.6", |
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.
Should these stay on next
?
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 the same but yeah — I'll remove them to reduce PR noise
@@ -1,97 +1,28 @@ | |||
/* @flow */ | |||
|
|||
const fs = require(`fs`) | |||
const path = require(`path`) | |||
const json5 = require(`json5`) |
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.
seems like we can remove json5
from package.json (this and tests you remove are only places where this is used)
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.
LGTM!
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.
🛩
nice! thanks for picking this up 👍 |
* WIP: flesh out how the babel-loader might work * WIP * Write out babel plugin info + stage for custom babel-loader * cleanups * Add missing await * Don't set babelrc to false since we check for `partialConfig.hasFilesystemConfig()` * Set env variable instead of writing out file * Remove old file for creating config * Add back stage specific plugins * Add required plugins/presets to babel config even if there's a file-defined schema * Update sample .babelrc people can copy into their site * Trim away more unused code * update snapshot * Add fallback plugins for when a .babelrc isn't defined + merge in gatsby plugin additions * Comments * Add back plugin * Update snapshots * update tests * Add some docs * test helper libs * Fix lint errors * Remove json5 from gatsby's package.json as it's unused now * mock resolve
VERY WIPI wanted to throw it up early tho to get feedback and make sure we are on the right track here.Edit by @m-allanson: This has been picked up by @KyleAMathews and is ready for review.
Closes #4689