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

lerna link convert → forced bad hoisting; bad file: refs #1419

Closed
garthk opened this issue May 10, 2018 · 9 comments
Closed

lerna link convert → forced bad hoisting; bad file: refs #1419

garthk opened this issue May 10, 2018 · 9 comments
Labels

Comments

@garthk
Copy link
Contributor

garthk commented May 10, 2018

After running lerna link convert with 3.0.0-beta.20:

  • Dependencies were hoisted despite hoist: false
  • References were added to the top level package.json to each package in the monorepo, which might have been intended
  • Those references were of the form file:../pkgname, which was definitely not intended as the path should have been pkg/pkgname

I wouldn't raise bugs against this behaviour if it were unreleased, but it's on npm and @evocateur is recommending people upgrade to work around other problems e.g. #1418, which is close enough to released that I figure it's worth the effort.

Expected Behavior

  • Changes to README to let me know what behaviour to expect
  • No hoisting if hoist: false
  • Hoisting still supported, as we rely on it to keep build time down on deployment packages
  • All file: references point to a valid path

Current Behavior

  • No docs
  • Hoisting despite hoist: false
  • file: references to packages in the monorepo are invalid at the top level

Steps to Reproduce (for bugs)

With node 8.11.1 and npm 5.6.0 (e.g. under the prompt you get from docker run node:8):

cd `mktemp -d`
mkdir repro
cd repro
echo node_modules > .gitignore
mkdir -p pkg/one
mkdir -p pkg/two
echo '{"lerna":"3.0.0","hoist":false,"packages":["pkg/*"],"version":"independent","command":{"publish":{"ignoreChanges":["*.md","test/**"]}}}' > lerna.json
echo '{"name":"repro","version":"1.0.0","devDependencies":{"lerna":"^3.0.0-beta.20"}}' > package.json
echo '{"name":"@example/one","version":"1.0.0","main":"index.js","devDependencies":{"tap":"^11.1.5"}}' > pkg/one/package.json
echo '{"name":"@example/two","version":"1.0.0","main":"index.js","devDependencies":{"tap":"^11.1.5"},"dependencies":{"@example/one":"^1.0.0"}}' > pkg/two/package.json
npm install
git init
git add .
git commit -m "before" 
node_modules/.bin/lerna link convert
git diff

Note:

  • tap added to the top level devDependencies
  • tap removed from devDependencies in pkg/one/package.json
  • tap removed from devDependencies in pkg/two/package.json
  • I couldn't reproduce the file:../pkgname problem, but submit the rest as proof-of-effort
lerna.json

{
  "lerna": "3.0.0",
  "hoist": false,
  "packages": [
    "pkg/*"
  ],
  "version": "independent",
  "command": {
    "publish": {
      "ignoreChanges": [
        "*.md",
        "test/**"
      ]
    }
  }
}

Context

We're still having trouble driving our monorepo with lerna, particularly around version management and preparing deployment packages:

  • Having to update every dependent's package.json to reflect minor and patch bumps spams our diffs, and things get awkward if we miss one. --force-local might help, here, while hurting us if it masks us forgetting a major bump.

  • If you want to prepare a deployment package without first publishing everything to npm, it seems obvious that all you need to do is set up a deployment package in the monorepo. Win! Now, try to get only the files you need for production into node_modules. Lose!

I was hoping I could solve the deployment problem by jamming paths into the version fields on my dependencies to get npm install --production in pkg/whatevs-deploy to prepare a local node_modules without symlinks for deployment, but hit an npm bug.

While researching the above, I tripped over #1418, which raised my hope that a lerna update might help us out. It didn't.

PS this is an awesome question to put in your issue template. Good on you.

Your Environment

macOS and Linux

Executable Version
lerna --version 3.0.0-beta.20
npm --version 5.6.0
node --version v8.11.0
@garthk
Copy link
Contributor Author

garthk commented Jun 13, 2018

Still hoisting in 3.0.0-beta.21.

@evocateur
Copy link
Member

  • "hoisting" is intentional, as npm install packages/foo does not install devDependencies anyway (just like a "normal" npm install)
  • "hoist": false is irrelevant
  • "foo": "file:packages/foo" is the intended root specifier, i've never hit that bad relative path in the root (over about a dozen conversions of internal monorepos in the past couple months)

sorry for the lack of docs

@stale
Copy link

stale bot commented Dec 27, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@eemeli
Copy link

eemeli commented Jan 21, 2019

With Lerna 3.10.6, running lerna link convert got me a few of the packages in both the dependencies and devDependencies of the top-level package.json; of these the dev dependencies had the incorrect file:../foo paths. Maybe related: This also removed the local dependencies from a few packages, which broke their build order.

Hoisting the packages' dev dependencies out of their package.json files also hoisted e.g. @babel/cli, which broke my package-specific build commands.

As an unexpected consequence of using file: paths, lerna bootstrap and lerna link no longer add symlinks to the packages' node_modules directories. This breaks e.g. node-sass v4, which requires using node_modules/foo/... paths rather than supporting ~foo/... for node-like path resolution.

Not really happy with this command. I tried making it work for a while, but ended up rolling back my changes. In case it matters, the packages in this repo are all scoped.

@evocateur
Copy link
Member

Yeah, I guess I never accounted for the "use local devDependencies to influence topological sort" case. Seems like maybe that loop should be aware of "am i local or not".

TBH, I don't think that's a very useful trick, as it is completely ignored during lerna publish. Build order shouldn't be predicated on which tools they depend on.

If you're using relative file: specifiers, you never need to use lerna bootstrap or lerna link again. Just npm install (or npm ci) in the root.

As for node-sass, that sounds like a tool that isn't using node's module resolution algorithm correctly. FWIW, file: specifiers create an almost identical root node_modules layout (symlinks to local packages, etc) that Yarn Workspaces do. Both patterns rely on proper resolution of node_modules directories.

@eemeli
Copy link

eemeli commented Jan 21, 2019

To clarify, the dev dependency in my case isn't a tool, but one package's built output that gets copied into and released as a part of another package. Therefore it's a dev (build-time) dependency, rather than a runtime dependency. In part this is needed to match the package's pre-monorepo structure, but it also makes node-sass much easier to use as its v5 release seems to be dragging on for quite some time.

Is this the right issue to also track the problems caused by the loss of package-specific node_modules/.bin/ folders?

@evocateur
Copy link
Member

Ah, ok. Interesting. The lack of local bins (or at the very least, local bin resolution) is a somewhat distinct issue. I've been noodling about it, probably need a root postinstall lerna link bins or something...

@stale
Copy link

stale bot commented Mar 22, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 22, 2019
@stale stale bot closed this as completed Mar 29, 2019
@evandeininger
Copy link

@evocateur have you given any thought to adding that lerna link bins script? so far I've been using a separate package called "link-parent-bin" that has had mixed results.

Also thanks for all the support you've given to this I know it's tough to do open source work and this really helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants