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

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

Closed
david-boles opened this issue Aug 5, 2019 · 5 comments
Closed

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

david-boles opened this issue Aug 5, 2019 · 5 comments

Comments

@david-boles
Copy link

david-boles commented Aug 5, 2019

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
Copy link
Contributor

bterlson commented Aug 6, 2019

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

@david-boles
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

bterlson commented Aug 7, 2019

@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 bterlson pinned this issue Aug 16, 2019
@bterlson bterlson unpinned this issue Aug 16, 2019
@bterlson
Copy link
Contributor

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.

bterlson added a commit that referenced this issue Aug 20, 2019
This addresses the confusion reported in #402 

IMO it would be nice to make this easier on people by supporting includeModule/excludeModule, but in the meantime we should document symlinks a bit better.
lukastaegert pushed a commit that referenced this issue Aug 22, 2019
* Update README.md with note on symlinks

This addresses the confusion reported in #402 

IMO it would be nice to make this easier on people by supporting includeModule/excludeModule, but in the meantime we should document symlinks a bit better.

* Couple cleanups

* Apply suggestions from code review

Co-Authored-By: Andrew Powell <[email protected]>

* Update namedExports docs

Prefer specifying module names over paths
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

2 participants