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

Doesn't include files in package installed from filesystem. #402

Closed david-boles closed 5 years ago

david-boles commented 5 years ago

With include set to ['node_modules/**'], it doesn't seem to process packages installed from the local filesystem (e.g. npm i ~/foo/bar). Given that installing packages that way copies them into node_modules, I believe this is a bug.

bterlson commented 5 years ago

@david476 can you upload a repro to a git repo (with the node_modules folder included so npm install is not needed)?

david-boles commented 5 years ago

I just realized that npm symlinks the folder, that's probably why; this may not actually qualify as a bug then. Anyway, here's a minimal example, just run npm run control and npm run buggy from within the package folder.

david-boles commented 5 years ago

Also, for future reference if people run into this, you can install your package from the filesystem using npm install $(npm pack path/to/package | tail -1).

bterlson commented 5 years ago

@david476 thanks so much for the repro. I think there is a real bug under the covers, though probably not on this project. @lukastaegert any thoughts here?

The issue is ultimately we're trying to filter realpaths (./library/index.js) against symlinked paths (./package/node_modules/library/index.js). This points to two fixes on your end, @david476:

  1. Pass "preserveSymlinks: true" to your rollup config, or:
  2. Update your filter to be based on the realpath rather than the symlinked path.

However, something seems amiss here - I'm not sure why rollup-plugin-node-resolve is realpathing itself here - shouldn't we just blindly use whatever resolve gives us? As far as resolve is concerned, preserveSymlinks: true is still the default behavior, which might be the cause of this mismatch...

bterlson commented 5 years ago

I have been working toward a goal of supporting include/exclude paths like node_modules/**/* when node_modules subdirectories are symlinked elsewhere e.g. with npm link. Unfortunately, I don't see how this can work without either:

  1. modifying rollup/rollup + browserify/resolve to resolve using both realpaths and symlinked paths so we can compare against either when doing the CJS transform, or
  2. modifying minimatch to support matching against realpaths of symlinks as well.

Neither solution seems palatable. I believe this is because we're comparing paths procured from rollup-plugin-node-resolve with paths procured from evaluating a glob expression, and the semantics of these are sufficiently different that they can't be unified. We would have been in a similar boat earlier trying to fix #400 if namedExports keys were globs rather than module names.

One possible improvement is to leave include/exclude as-is (but better documented) and also support an includeModules/excludeModules form that is a list of module names that are resolved using broserify/resolve with the user-provided preserveSymlinks setting. So { include: ['node_modules/buffer/**'] } could instead be expressed as { includeModule: ['buffer']} with the added benefit that this works regardless of symlinks and the preserveSymlinks configuration.

Closing this issue now, but feel free to re-open if someone can think of how to make a glob like node_modules/** work when node_modules are symlinked.