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
fix: add regenerate-unicode-properties to dynamicRequireTargets #12819
Conversation
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 eb907f0:
|
Gulpfile.mjs
Outdated
@@ -292,6 +292,10 @@ function buildRollup(packages, targetBrowsers) { | |||
"packages/babel-compat-data/*.js", | |||
"packages/*/src/**/*.cjs", | |||
], | |||
dynamicRequireTargets: [ | |||
// https://github.com/mathiasbynens/regexpu-core/blob/ffd8fff2e31f4597f6fdfee75d5ac1c5c8111ec3/rewrite-pattern.js#L48 | |||
"node_modules/regenerate-unicode-properties/**", |
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 guaranteed that it's always inside the top-level node_modules
, since it's a nested dependency.
WDYT about creating a utility like this?
import { createRequire } from "module";
import { fileURLToPath } from "url";
import { dirname } from "path";
/**
* Resolve a nested dependency starting from the given file
*/
export default function resolveChain(baseUrl, ...packages) {
const require = createRequire(baseUrl);
return packages.reduce(
(base, pkg) => require.resolve(pkg, { paths: [dirname(base)] }),
fileURLToPath(baseUrl)
);
}
And then we can use
resolveChain(
import.meta.url,
"packages/babel-helper-create-regexp-features-plugin",
"regexpu-core",
"regenerate-unicode-properties"
);
Also, maybe we can end the pattern with **/*.js
to only include JS files?
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.
Thanks! That looks fancy! I think it is okay to use **/*
. If we have require("foo/bar.md")
, rollup will throw when bundling bar.md
.
I think resolveChain
will break if a package switch main
from index.js
to lib/index.js
since we are meant to match all files within this package.
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.
We can use regenerate-unicode-properties/package.json
as the last param to avoid that breakage.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/41157/ |
The e2e error is fixed in #12821 , will rebase once that one gets merged. |
Co-Authored-By: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
This seems to work locally because it delegates to Node's |
The confusing error unknown
L
is thrown because Rollup complainsregenerate-unicode-properties
should be added todynamicRequireTargets
, which is then caught inhttps://github.com/mathiasbynens/regexpu-core/blob/ffd8fff2e31f4597f6fdfee75d5ac1c5c8111ec3/rewrite-pattern.js#L64
resulting to a complete different error message.