Skip to content
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

feat(commonjs): Add ignore-dynamic-requires option #819

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

lukastaegert
Copy link
Member

Rollup Plugin Name: commonjs

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • 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

As I am doing more Node bundling, I sometimes want to completely externalise a dependency (because it is doing evil things) but do so in a way that the exact execution order is preserved. So what I want is to keep an untranspiled require statement. This works well for static requires but is just not possible for dynamic require statements EXCEPT if you use a loophole: If you specify some dynamicRequireTargets but do not use them, the dynamic require function injected will do an actual require as fallback.

Now this is not nice for two reasons:

  • I do not want the overhead of dynamicRequireTargets, and it is also weird that I have to specify an actually existing file I do not use to trigger the use of the enhanced require function
  • IF I want to use dynamicRequireTargets, then I have no feedback if I really did it correctly. Why? Because when testing locally, the existing node_modules will gladly "patch over" any dependencies I forgot. But when I deploy my bundled code without the adjacent node_modules, it will be broken unexpectedly.

The goal of this PR is to make this a conscious decision:

  • if ignoreDynamicRequires is set to true, dynamic requires will either be left untouched, or if dynamicRequireTargets is used, will get the current require fallback
  • if ignoreDynamicRequires is set to false or left unspecified, then dynamic requires will always throw an error if they cannot be resolved via dynamicRequireTargets.

You already got this error before without the dynamicRequireTargets option, the breaking change is that you now also get it when dynamicRequireTargets is specified. I feel this is much more consistent and hopefully worth the break. We could make it non-breaking by changing the default, though. On the other hand, maybe we can couple this with #658?

The naming choice was made to extend the ignore option that does the same for static require calls.

Possible future extensions that I would see as out-of-scope for now but we could easily add if there is demand:

  • allow string values to "replace" the require function with another global specified?
  • allow a special value ignoreDynamicRequires: "stub" that replaces unresolved dynamic require calls with an empty object? This might be the more useful version, that way, tree-shaking could later easily clean up these calls without side-effects. But again, only if there is actual demand.

BREAKING CHANGES:
When dynamicRequireTargets is specified, "require" will be default no longer be used as fallback
@lukastaegert
Copy link
Member Author

With regard to the coverage issues, maybe we want to switch to the official codecov instead of codecov-lite package? The latter does not seem to be too well maintained.

Copy link
Contributor

@danielgindi danielgindi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Member

@tjenkinson tjenkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! The new behaviour is clearer and worth the breaking change IMO, especially given if it does break for someone it’s a simple fix

@shellscape shellscape merged commit 2d06c44 into master Mar 10, 2021
@shellscape shellscape deleted the feat/commonjs/keep-dynamic-requires branch March 10, 2021 15:23
patak-dev pushed a commit to vitejs/vite that referenced this pull request May 11, 2021
- v18 added the `ignoreDynamicRequires` option as a breaking change. The earlier behaviour matched `true`, so I set it as the default (ref: rollup/plugins#819)
- v19 is a breaking change since it now requires Rollup 2.38.0, but we’re already on 2.38.5. (ref: rollup/plugins#658)
fi3ework pushed a commit to fi3ework/vite that referenced this pull request May 22, 2021
- v18 added the `ignoreDynamicRequires` option as a breaking change. The earlier behaviour matched `true`, so I set it as the default (ref: rollup/plugins#819)
- v19 is a breaking change since it now requires Rollup 2.38.0, but we’re already on 2.38.5. (ref: rollup/plugins#658)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants