Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

[createFilter] Add an option to include virtual modules? #54

Closed
eight04 opened this issue Mar 4, 2019 · 6 comments
Closed

[createFilter] Add an option to include virtual modules? #54

eight04 opened this issue Mar 4, 2019 · 6 comments

Comments

@eight04
Copy link

eight04 commented Mar 4, 2019

Currently, virtual modules are excluded by default. This prevents external-globals plugin from processing proxy modules generated by commonjs plugin. How do we handle this situation?

  1. Don't use the filter and include all files since the plugin should work for every module?
  2. Don't ignore virtual modules but still respect user's include/exclude config?
@lukastaegert
Copy link
Member

Could you check if #61 solves this for you (by specifying resolve: false)?

@eight04
Copy link
Author

eight04 commented May 14, 2019

@lukastaegert IIRC, vue plugin uses something like path/to/file.js?generated-by-vue as the module ID. Will it be a better alternative to \0proxy-module:path/to/file.js?

Anyway, if commonjs decide to change their ID and the ID can be processed by other plugins, this shouldn't be an issue anymore. Since it is intended to prevent virtual modules from being processed by plugins according to the doc:

If your plugin uses 'virtual modules' (e.g. for helper functions), prefix the module ID with \0. This prevents other plugins from trying to process it.

This issue probably shouldn't be handled by #61.

@lukastaegert
Copy link
Member

The common-js proxy modules contain hardly any code and the plugin relies on the fact that no other plugins mess with this code. Therefore I would be very careful to remove the \0 from the beginning of the id, I am wary here about plugins that e.g. inject new imports into each file. However we could change the rest of the ID to use the VUE way safely for now.

For the issue with the globals plugin, this is a special case in that we rely on this specific behaviour of rollup-plugin-commonjs to translate require statements to something we understand.

So maybe (once we changed rollup-plugin-commonjs), rollup-plugin-globals could handle this the following way:

  • Ids starting with a \0 are included by default (maybe add an option to disable this behaviour if you foresee issues)
  • if include/exclude is provided, check if the id starts with a \0 and if so, remove the \0 first before passing it through the filter

@lukastaegert
Copy link
Member

With this comment, maybe I revert the change to allow ids starting with a \0 to be allowed by createFilter and rather rely on plugin-authors to manually remove the \0 from each id before passing it to createFilter if they think it makes sense for them to also transform plugin-specific modules.

@lukastaegert
Copy link
Member

I have changed the commonjs proxy format in rollup/rollup-plugin-commonjs#387 and removed the automatic resolution of private ids again from #61

@shellscape
Copy link
Contributor

We've moved this repo to https://github.com/rollup/plugins. If anyone would like to re-submit this issue there, we'd welcome it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants