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

node_modules isn't cool anymore, what to do #29

Closed
dominic-p opened this issue Oct 6, 2018 · 12 comments
Closed

node_modules isn't cool anymore, what to do #29

dominic-p opened this issue Oct 6, 2018 · 12 comments

Comments

@dominic-p
Copy link

I just read about yarn's new plug n play install and it looks like npm might be pursuing something similar.

Since one of the main heuristics used here is the presence of node_modules, I could see this breaking if these new module resolution systems gain traction.

@afzalah
Copy link

afzalah commented Oct 8, 2018

I guess it's like doing the same thing over and over again, why yarn doesn't simply embed/use @verdaccio and support in their development instead.

@inxilpro
Copy link
Owner

inxilpro commented Oct 8, 2018

Hm, yeah. It seems like we'll need new heuristics. Sadly, it looks like PnP will fundamentally break the way this library works…

Luckily, our fallback strategy of using require.main.filename should continue to work in most cases, but it's not ideal.

@dominic-p
Copy link
Author

Yeah, that's true. I have played around a bit with require.main and it's definitely not bullet proof, especially when running tests.

@inxilpro
Copy link
Owner

inxilpro commented Oct 8, 2018

Yeah, test-runners and anything else that inject themselves before your app will break that method. We may need to just check for those cases and rely on require.main more heavily, though…

@arcanis
Copy link

arcanis commented Oct 8, 2018

FYI with PnP it should just be a matter of doing this:

const pnp = process.versions.pnp
  ? require('pnpapi')
  : null;

const appRoot = pnp
  ? pnp.getPackageInformation(pnp.topLevel).packageLocation
  : null;

@benface
Copy link

benface commented Nov 4, 2018

This package seems to be checking for package.json instead of node_modules: https://github.com/sindresorhus/pkg-dir

Would that work? Is it too simple/naive and doesn't work in some cases?

@dominic-p
Copy link
Author

Good suggestion @benface. I've played around with looking for package.json. It seems a bit less robust than node_modules in my testing.

For example, if you are in a path inside node_modules, you might assume your current directory is the root because it has a package.json file when really you want the parent directory (the one that has node_modules in it).

For example:

/some/path/project/node_modules/mocha/package.json
/some/path/project/package.json

@benface
Copy link

benface commented Nov 5, 2018

Oh, ok so it wouldn't work for task runners / test frameworks because those run from their own directory?

@arcanis
Copy link

arcanis commented Nov 5, 2018

Note that the Yarn cache is (in most cases for now) outside of the project folder. So if you simply traverse upwards until you find the top-most node_modules / package.json, the only one you'll find are your own.

For a perfect integration, I recommend my snippet above 🙂

@Vanuan
Copy link

Vanuan commented Mar 5, 2019

Another thing to consider is that yarn allows to override node_modules location:

# .yarnrc file in project root
--modules-folder /path/to/node_modules/elsewhere

I'm disappointed. I thought app-root-path is doing something smarter than making assumptions about its own location in a filesystem.

@Vanuan
Copy link

Vanuan commented Mar 5, 2019

Look here:
lint-staged/lint-staged#477

@inxilpro
Copy link
Owner

Thanks @arcanis — 2.2.0 adds pnp support.

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

No branches or pull requests

6 participants