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

Hoisting breaks `require` semantics #867

Open
ef4 opened this Issue Jun 6, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@ef4

ef4 commented Jun 6, 2017

Hoisting is intended as a runtime optimization that doesn't affect program correctness. But it's easy to accidentally a write a package that only works when hoisting is in effect that then breaks if deployed as a normal package.

Expected Behavior

Each package should have access to the exact same set of dependencies, whether or not hoisting is applied.

Current Behavior

Hoisted dependencies leak into packages that otherwise should not have access to them.

Possible Solution

Move the hoisted dependencies out of the natural node module resolution path and instead link them explicitly into the packages where they are appropriate.

For example, assume that first and second share my-hoisted-dep. Today we will get this, which leaks my-hoisted-dep into third:

├── node_modules
│   └── my-hoisted-dep
└── packages
    ├── first
    ├── second
    └── third

Instead we could move the hoisted deps off the module resolution path:

├── lerna-common
│   └── node_modules
│       └── my-hoisted-dep
└── packages
    ├── first
    │   └── node_modules
    │       └── my-hoisted-dep -> ../../../lerna-common/node_modules/my-hoisted-dep
    ├── second
    │   └── node_modules
    │       └── my-hoisted-dep -> ../../../lerna-common/node_modules/my-hoisted-dep
    └── third

This would also eliminate the need for npm --global-style, which is an incomplete solution to this same general problem (it replaces wrong versions with correct versions, but won't replace wrong versions with "no version").

Context

Moving a piece of code between packages is a reasonably common thing to do. If you do that while using hoisting, it's easy to produce packages that no longer work standalone without realizing it. For this reason, IMO it's not currently safe to use --hoist, especially in environments like CI, since it will let this class of bug escape.

@evocateur

This comment has been minimized.

Show comment
Hide comment
@evocateur

evocateur Jun 6, 2017

Member

I would argue that's what eslint-plugin-import is for (specifically import/no-unresolved).

Member

evocateur commented Jun 6, 2017

I would argue that's what eslint-plugin-import is for (specifically import/no-unresolved).

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 6, 2017

This situation is just always true regardless of whether you use lerna. If you npm install a package it's deps are installed alongside it in a flat structure in node_modules and are resolvable via require regardless of whether you depend on it directly. Lerna's hoisting behavior isn't anymore incorrect, in what it allows you to resolve than plain use of npm. I agree that eslint rules are the better way to avoid such things.

jquense commented Jul 6, 2017

This situation is just always true regardless of whether you use lerna. If you npm install a package it's deps are installed alongside it in a flat structure in node_modules and are resolvable via require regardless of whether you depend on it directly. Lerna's hoisting behavior isn't anymore incorrect, in what it allows you to resolve than plain use of npm. I agree that eslint rules are the better way to avoid such things.

@trusktr

This comment has been minimized.

Show comment
Hide comment
@trusktr

trusktr Feb 6, 2018

Lerna's hoisting behavior isn't anymore incorrect, in what it allows you to resolve than plain use of npm.

Not true. In a non-monorepo collection of projects, one normally goes and installs dependencies for each project separately. One project cannot import the dependencies from another project.

If the npm user were to go an npm install each project's dependencies from their home folder (this, installing at ~/node_modules, then, this might cause the problem, but people are very unlikely to do that.

When Lerna hoists dependencies, it makes dependencies visible to all packages, which just isn't very safe, and is not a common scenario at all. Most people install dependencies only inside their project and thus don't have this problem, while with Lerna hoisting they do have this problem.

trusktr commented Feb 6, 2018

Lerna's hoisting behavior isn't anymore incorrect, in what it allows you to resolve than plain use of npm.

Not true. In a non-monorepo collection of projects, one normally goes and installs dependencies for each project separately. One project cannot import the dependencies from another project.

If the npm user were to go an npm install each project's dependencies from their home folder (this, installing at ~/node_modules, then, this might cause the problem, but people are very unlikely to do that.

When Lerna hoists dependencies, it makes dependencies visible to all packages, which just isn't very safe, and is not a common scenario at all. Most people install dependencies only inside their project and thus don't have this problem, while with Lerna hoisting they do have this problem.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Feb 6, 2018

One project cannot import the dependencies from another project

That's not true, you can import nearly every transitive dependency regardless of whether you depend on it directly just look in node_modules on a single package and try, yes Lerna theoretically adds more you can accidentally require but it's not more unsafe than any project that uses npm or yarn

jquense commented Feb 6, 2018

One project cannot import the dependencies from another project

That's not true, you can import nearly every transitive dependency regardless of whether you depend on it directly just look in node_modules on a single package and try, yes Lerna theoretically adds more you can accidentally require but it's not more unsafe than any project that uses npm or yarn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment