search

rollup / rollup-plugin-commonjs

This module has moved and is now available at @rollup/plugin-commonjs / https://github.com/rollup/plugins/blob/master/packages/commonjs
MIT License
502 stars 126 forks source link

[Do Not Merge] Different strategy for named export mapping (fixes #419) #420

Closed bterlson closed 4 years ago

bterlson commented 4 years ago

419 observes that a named exports field like:

namedExports: {
  "jquery": ...
}

Won't work right when jquery is not a direct dependency and therefore might not be present in node_modules in some package managers, or when multiple copies of jquery exist in your compilation.

This PR addresses the problem by not mapping module names to paths up front. Instead, the module name itself is preserved in the customNamedExports map, along with the path.resolve'd path and the realpath (if different). Later during transform, the package scope for each module id is found, its package.json name field is read, and looked up in customNamedExports. If an entry is found, that is used, else the mapping for the id as-is is used.

On the plus side, we no longer have a dependency on browserify/resolve and do less work up front. On the minus side, we're doing a lot more work during transform, and so I expect on the balance this will be a performance regression. To me, the trade-off seems worth it, but I'll leave the final call to others to decide.

Further work if this overall approach is acceptable: map only the module's entrypoints (main, and browser or browser-mapped main). Right now all tests pass as is but this doesn't work for cjs modules which import other cjs modules.

bterlson commented 4 years ago

Updated to take advantage of https://github.com/rollup/rollup-plugin-node-resolve/pull/244. Tests are now failing as in the current implementation, if you do not have the latest rollup-plugin-node-resolve, you cannot use namedExports keys that are module names. Couple options:

  1. Take the breaking change and document the requirement for node-resolve when you want to specify named exports by module name
  2. Restore the browserify/resolve dependency and old behavior when initializing customNamedExports so cases where we can't get a package name from a package.json work like they did before by looking up ids in the customNamedExports map.

Curious if anyone has preferences here.

shellscape commented 4 years ago

@bterlson we're gearing up to move this plugin to rollup/plugins soon and are trying to tie off loose ends. how would you like to proceed with this PR?

bterlson commented 4 years ago

Since the changes here aren't too extensive, I'm ok closing this and rebuilding it again from its new location.