plugins icon indicating copy to clipboard operation
plugins copied to clipboard

WIP: Support bare imports in @rollup/plugin-dynamic-import-vars

Open sventschui opened this issue 2 years ago • 1 comments

Rollup Plugin Name: dynamic-import-vars

This PR contains:

  • [ ] bugfix
  • [x] feature
  • [ ] refactor
  • (not yet) documentation
  • [ ] other

Are tests included?

  • [ ] yes (bugfixes and features will not be merged without tests)
  • [ ] no
  • [x] not yet

Breaking Changes?

  • [ ] yes (breaking changes will not be merged unless absolutely necessary)
  • [x] no

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:

Description

This Draft PR adds two things I'd like to discuss with you before iterating further on this. The code is not yet completely polished and test are missing. Happy to get your feedback on the thoughts before I iterate any further on this (adding tests, documentation, improving code, ...).

Introduce a way to ignore certain imports

We're currently using webpack and it's webpackIgnore: true comment on some imports we wish to run in the browser, i.e. import(/* webpackIgnore: true */ someVariablePointingToAnHttpUrl);. This is used to load modules in our proprietary microfrontend framework (think module-federation). Since we now want to bundle certain parts of our production application with rollup instead of webpack (as the webpack ESM output is sub-optimal) we need to support these runtime-dynamic-imports in our rollup builds as well.

I resorted to support the magic commentwebpackIgnore: true on dynamic imports in order to not transform them with the dynamic-import-vars plugin. I think it could make sense to introduce a rollup or rollup/plugin-dynamic-import-vars specific magic comment in order to not break any existing builds.

Add support for bare module imports with variables

Currently the plugin does not support bare imports as they're a bit hard to analyze. As we're using dynamic imports to i18n-iso-countries to dynamically load the country names in different languages, we're in need to support dynamic variable imports with bare specifiers. (Alternative would be to copy the files to our src folder but I dislike this very much as it would not de-dupe properly when the same library is used in other parts (i.e. inside node_modules)).

To support bare import specifiers I check the generated glob (not starting with ./ nor ../) whether it starts with an npm package name (basically @scope/name or name) using a regexp. If a package name could be found, then <name>/package.json is resolve()d in order to locate the npm package. The glob is then adjusted to use the resolved location instead of the bare import specifier. In the end this has to be reversed in the switch statement generated.

Sub-optimal changes to dynamic-import-to-glob

The code is not very optimal yet and I had to make some changes to dynamic-import-to-glob I somewhat dislike:

  • make it async as resolve() is async
  • pass the rollup instance in order to access resolve()

I tried to work around this by moving the resolve() parts into the index.js file but this has other drawbacks:

  • throwing VariableDynamicImportError in index.js instead of dynamic-import-to-glob.js

Happy to suggestions for this

sventschui avatar Aug 31 '22 06:08 sventschui

@shellscape as github mentions you as a potential reviewer would you mind looking at the description of this PR to guide me on how to continue here?

sventschui avatar Sep 01 '22 13:09 sventschui

I ran into the same issue when trying to build a lib which can make use of import maps in the browser. @shellscape Any chance on getting this into a release any time soon?

matoilic avatar Oct 03 '22 13:10 matoilic

Closing as abandoned.

shellscape avatar Nov 27 '22 17:11 shellscape