fix(dynamic-import-vars): keep dynamic import assert arg intact #1504
Conversation
jorenbroekema
force-pushed
the
import-assertions-dynamic
branch
from
f168160
to
599a636
Compare
| ## Import Assertions | ||
|
|
||
| This plugin will keep your import assertions inside dynamic import statements intact, provided that you add the `acorn-import-assertions` syntax plugin to your Rollup config: | ||
|
|
||
| ```js | ||
| import { importAssertions } from 'acorn-import-assertions'; | ||
| import dynamicImportVars from '@rollup/plugin-dynamic-import-vars'; | ||
|
|
||
| export default { | ||
| plugins: [dynamicImportVars()], | ||
| acornInjectPlugins: [importAssertions] | ||
| }; | ||
| ``` | ||
|
|
||
| Will ensure the following import assertions remain intact: | ||
|
|
||
| ```js | ||
| // Refer to rollup-plugin-import-css https://github.com/jleeson/rollup-plugin-import-css | ||
| function importLocale(sheet) { | ||
| return import(`./styles/${sheet}.css`, { assert: { type: 'css' } }); | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This should not be needed as Rollup includes that plugin by default: https://github.com/rollup/rollup/blob/6797d576bf5b221e6592882d8d905da8dc978479/src/utils/options/normalizeInputOptions.ts#L2-L2
Did you try this? Does updating the Rollup version in the tests help?
There was a problem hiding this comment.
You're right, it works without needing to add it manually. I guess I based that off of some other plugin that was based on an older version of Rollup where this was still required.
|
@jorenbroekema please give the replies here a look 🙏 if we reach 60 days without a reply, we'll consider the PR to be abandoned. |
jorenbroekema
force-pushed
the
import-assertions-dynamic
branch
from
599a636
to
12a03cd
Compare
jorenbroekema
force-pushed
the
import-assertions-dynamic
branch
from
12a03cd
to
9b647da
Compare
|
Thanks for the reminder @shellscape , I addressed your comments, I might still need a hand on the test, the snapshot output seems to show the assert arg not appearing in the output, I'm not sure that's intended and I'm missing something, or my test is incorrect? Again, locally I tested this patch again, and it still does fix my issue, but I guess it would be nice if the test I am adding is a working regression test for my issue and not just a light smoke test |
There was a problem hiding this comment.
I think that test is sufficient for now. Thanks for adding it. @lukastaegert please have another look.
|
Sorry about the delay getting this merged. Thanks again! |
Rollup Plugin Name:
dynamic-import-varsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: #1503
Description
When transforming dynamic imports with variables in them, the second argument (import assertions) is stripped away and this information is lost. This Pull Request addresses that problem (see issue) by grabbing the argument Node and stringifying it and injecting it into the Magic String, so that this argument is kept as is.
This is needed for other plugins to correctly deal with imports that have import assertions, most notably https://github.com/jleeson/rollup-plugin-import-css which for CSS imports returns either a CSS string or CSSStyleSheet depending on whether or not an import assertion for CSS is present. If such an assertion is present, but dynamic-import-vars strips it away, the CSS plugin will bundle the CSS import as a CSS string rather than CSSStyleSheet, which would be different from local development, before bundling, since browsers natively do support import assertions now and return a CSSStyleSheet.
I might need some help with the testing, as it currently doesn't verify that the assertion is kept intact, I'm not sure how to test that, it seems in the final output[0].code the assertions are gone again, but when I test my changes in my local project, it does fix my issue.. and when I console.log the transform hook in this plugin with my changes, it does seem like it returns the correct code.