Warn when the version specified in transform-runtime doesn't match #10325
Conversation
nicolo-ribaudo
added
PR: New Feature 🚀
| const pkg = JSON.parse(fs.readFileSync(pkgFilename, "utf8")); | ||
|
|
||
| return ( | ||
| pkg.dependencies?.[moduleName] || |
There was a problem hiding this comment.
It is stage 3 now 🙏
nicolo-ribaudo
force-pushed
the
runtime-version-warning
branch
from
1630b5e
to
143eb28
Compare
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11330/ |
nicolo-ribaudo
force-pushed
the
runtime-version-warning
branch
from
143eb28
to
3dbae24
Compare
|
Rebased to include #10326 |
packages/babel-plugin-transform-runtime/test/fixtures/absoluteRuntime/relative/output.js
Show resolved
Hide resolved
| basedir: dirname, | ||
| }), | ||
| ); | ||
| const pkgFilename = path.resolve(runtimeDir, "../../../package.json"); |
There was a problem hiding this comment.
Hmm, I'm not sure this is what I'd recommend for verifying this. Could we consider looking for the package.json in a parent directory starting from dirname? It seems like treating the root as relative to the plugin usage location might be safer than using the location of the plugin, since that could be in an unknown place.
| const minActualVersion = semver.minVersion(actualRuntimeVersion).version; | ||
| const minVersion = semver.minVersion(runtimeVersion).version; | ||
|
|
||
| if (minActualVersion === minVersion) return; |
There was a problem hiding this comment.
Treating semver ranges as anything other than ranges is asking for trouble I think. Avoiding logic like this is why I lean toward intersects for things in
What we really want to know in this context is whether every possible version in actualRuntimeVersion is contained within runtimeVersion, I think? As in, every possible version of the runtime that could be installed by this package.json is valid given the semver range passed to the Babel plugin.
This current logic would pass if for instance we had
runtimeVersion: "^7.2.0",
actualRuntimeVersion: "~7.2.0"
since the minimum for both is 7.2.0.
I'm not sure if the semver module actually exposes and API for doing the comparison we need to do here, but we can certainly check.
| ).replace(/^/gm, " ".repeat(2)); | ||
|
|
||
| if (semver.gt(minActualVersion, minVersion)) { | ||
| console.warn( |
There was a problem hiding this comment.
I'd be very hesitant to add new logging to a plugin in a non-major if it isn't opt-in. Some tools will treat any stderr output as an indication that something in the build has failed.
I think the concern here is valid, @nicolo-ribaudo Can you rebase this one onto |
We can't automatically infer the version of
@babel/runtime, because there is a too high risk of introducing a breaking change by inferring the wrong version because of a dependency depending on@babel/runtime, but we can at least warn explaining the problem.I suspect that currently almost everyone isn't using the
verisonoption, leading to many helpers to be inlined.I will extract e700f31 to another PR, since it will be useful for testing other warning messages, to clean up our tests console output and to remove the distinction between fixtures and debug-fixtures in preset-env.