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 native Node.js functions when available #12458
Use native Node.js functions when available #12458
Conversation
}, | ||
// require.resolve's paths option has been introduced in Node.js 8.9 | ||
// https://nodejs.org/api/modules.html#modules_require_resolve_request_options | ||
replacement: template({ syntacticPlaceholders: true })` |
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 polyfill was already there, I only moved it.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/34336/ |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ffe2c04:
|
I opened this PR because I already had it ready, but if you don't think that the added co.plexoty in |
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.
so this doesn't remove the dep, just makes a plugin that toggles on or off the require to the dep based on node version - sounds fine since we will remove in v8 - and CI test should cover it? assuming that this shouldn't break anything in a patch
Exactly, we are testing both the behaviors: on Node.js > 10 we test the new built-in functions, while the old functions are tested on Node.js 6 and 8. The dependency is still there (because of the polyfill), but this PR makes it so that we will only have to update |
This is the same as #12439 but for
fs.mkdirSync(path, { recursive: true })
and forprocess.allowedNodeEnvironmentFlags
: we use the native implementation when available, and fallback to the polyfill when it's not.This allows us to more easily prepare a Babel 8 prerelease, in the spirit of #12440, by simply toggling the
BABEL_8_BREAKING
env variable.fs.mkdirSync(filepath, { recursive: true })
is polyfilled at build time asparseFloat(process.versions.node) >= 10.12 ? fs.mkdirSync : require("make-dir").sync
(we can't easily feature-check because we need to polyfill therecursive
option)process.allowedNodeEnvironmentFlags
is polyfilled asprocess.allowedNodeEnvironmentFlags || require("node-environment-flags")
.Note that during local development those polyfills aren't injected since they aren't needed, but we are testing them on CI (with Node.js 6 and 8).