-
Notifications
You must be signed in to change notification settings - Fork 373
[ Vite ] Regroup preserve-*-loaders-imports Vite plugin inside a vite-extension
#3002
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
base: trunk
Are you sure you want to change the base?
Conversation
packages/php-wasm/web/vite.config.ts
Outdated
| { | ||
| regex: /php_\d_\d\.js$/, | ||
| transform: (specifier) => | ||
| `../${specifier.split('/').slice(-3).join('/')}`, |
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 easy to parse just by looking at it, let's document these with examples of paths we're targeting with this expression
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. But I don't know if this became more understandable...
packages/php-wasm/web/vite.config.ts
Outdated
| { | ||
| regex: /intl\.so$/, | ||
| transform: (specifier) => | ||
| `../../../${specifier.split('/').slice(-6).join('/')}`, |
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.
ditto here
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.
packages/php-wasm/web/vite.config.ts
Outdated
| { | ||
| regex: /icu\.dat$/, | ||
| transform: (specifier) => | ||
| `../../../${specifier.split('/').slice(-2).join('/')}`, |
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.
ditto here
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.
| if (command !== 'build' || typeof specifier !== 'string') return; | ||
|
|
||
| for (const rule of rules) { | ||
| if (rule.regex.test(specifier)) { |
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.
What would it take to introduce a poka-yoke and throw an error when a rule matches zero specifiers?
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 could add a const matchedRules = new Set<PreserveLoadersRule>(); and add a matching rule :
if (rule.regex.test(specifier)) {
matchedRules.add(rule);
...And during buildEnd we could throw an error if a rule has not been specified :
buildEnd() {
if (command !== 'build') return;
const unusedRules = rules.filter(rule => !matchedRules.has(rule));
if (unusedRules.length > 0) {
const details = unusedRules
.map((rule) => `- ${rule.regex}`)
.join('\n');
this.error(
`vite-preserve-loaders-imports: The following rules did not match any dynamic imports:\n${details}\n\n` +
`This is likely a misconfiguration or a stale regex.`
);
}
},
But I think if some regex were not correct Vite would crash anyway.
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 probably would! Still, having an explicit line of defense is always reassuring – we can stay resilient even if something implicit changes and we never realize. Let's add that buildEnd() logic.
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! Thank you Adam.
bec8e01 to
921dfb0
Compare
packages/php-wasm/web/vite.config.ts
Outdated
| * web/src/lib/get-php-loader-module.ts > web/src/get-php-loader-module.ts | ||
| * | ||
| * slice(-3) strips the `public` directory from the path | ||
| * web/public/php/jspi/php_8_4.js > ./web/php/jspi/php_8_4.js |
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.
"web/public/php/jspi/php_8_4.js".split('/').slice(-3).join('/')yields php/jspi/php_8_4.js, not ./web/php/jspi/php_8_4.js. Let's double check all the examples are accurate
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 exactly proves why these comments are important 😄. I corrected them and added a "resulting in" paragraph.
Done.
Motivation for the change, related issues
Based on #2999
Intl dynamic extension added a third file where its import had to be ignored : icu.dat. The pull request created a third
preserve-data-loaders-importsVite plugin. This pull request aims to regroup every preserve-{extension}-loaders-imports into a unique Vite extension.This pull request is more a suggestion than its
ignore-*-importscounterpart.Implementation details
Regrouping repeatedly copy pasted vite plugins inside a vite extension file named `vite-preserve-loaders-imports.ts.
Testing Instructions
CI -
test-built-npm-packagesshould not fail