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

[BUG] <title> #3743

Closed
1 task done
craigphicks opened this issue Sep 12, 2021 · 2 comments
Closed
1 task done

[BUG] <title> #3743

craigphicks opened this issue Sep 12, 2021 · 2 comments
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release

Comments

@craigphicks
Copy link

craigphicks commented Sep 12, 2021

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Issue #2035 was closed with an example of how to install own projects a and b and make 'a' depend upon 'b'.
That doesn't work (anymore?). The reason is that b already exists on npm, but despite that the installation half-works, and reason for failure is not clearly informed through error message. (E.g., Project 'a' also exists on npm, and that does NOT cause an error.)

Expected Behavior

If project 'x' can be initialized in a workspace, then thereafter that name should take precedence over an npm package with same name, when used in the workspace context.

Notice the described expected behavior includes the possibility of refusing to initialize a project because the name is already in the npm namespace. However it think it would be better to allow the any name for local use, and worry about naming collision later, when it comes time to publish to npm.

Why?

  1. So users can play with a/b type examples to learn workspaces.
  2. Otherwise in a workspace project, the only safe thing to do commit a dummy project for every workspace name. Not only is that time consuming (so most people will skip that), but it will result in a lot of unused npm project names as workspace projects may be merged, renamed, or removed during development. *(If a name is NOT reserved on npm , then during local development, then current behavior would allow an unexpected foreign (and possibly hostile) tounintentionally be installed under a workspace. As shown in my example reproduction, npm allows it in a way that might go unnoticed.)

Supposing npm does not want to let workspace names have precendece over npm package names. Then using file names (specifically the file name of the target package) should be possible as a workaround. Currently it seems not possible, as shown in the note test behaviour described below.

Steps To Reproduce

Here's what I get when I run the #2035 a/b example

$ npm init -y ; npm init -w packages/a -y ; npm init -w packages/b -y ;
...
$ npm i b -w a

added 2 packages, and audited 4 packages in 530ms

found 0 vulnerabilities

then

$ npm ls
[email protected] /home/craig/tmp
├─┬ [email protected] -> ./packages/a
│ └── [email protected]
└── UNMET DEPENDENCY b@file:/home/craig/tmp/packages/b

npm ERR! code ELSPROBLEMS
npm ERR! missing: b@file:/home/craig/tmp/packages/b, required by [email protected]

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/craig/.npm/_logs/2021-09-12T17_17_50_164Z-debug.log

Huh? I'll fix that:

$ npm i --save
added 1 package, and audited 6 packages in 389ms
found 0 vulnerabilities
$npm ls 
[email protected] /home/craig/tmp
├─┬ [email protected] -> ./packages/a
│ └── [email protected]
└── [email protected] -> ./packages/b

So it works!? I could almost let that slip by.

$ tree .
.
├── node_modules
│   └── a -> ../packages/a
├── package.json
├── package-lock.json
└── packages
    ├── a
    │   ├── node_modules
    │   │   └── b
    │   │       ├── examples
    │   │       │   └── index.js
    │   │       ├── History.md
    │   │       ├── lib
    │   │       │   ├── benchmark.js
    │   │       │   ├── b.js
    │   │       │   └── reporters
    │   │       │       ├── cli.js
    │   │       │       └── json.js
    │   │       ├── LICENSE
    │   │       ├── Makefile
    │   │       ├── package.json
    │   │       ├── Readme.md
    │   │       └── test
    │   │           ├── benchmark.test.js
    │   │           ├── b.test.js
    │   │           ├── reporter.test.js
    │   │           └── support
    │   │               ├── fake_stream.js
    │   │               └── test_reporter.js
    │   └── package.json
    └── b
        └── package.json

12 directories, 19 files

Package b has already been a package on npm for 8 years.
If I start again and the try to install b as a folder

$ npm i file://$(pwd)/packages/b -w a
npm ERR! Cannot set properties of null (setting 'dev')
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/craig/.npm/_logs/2021-09-12T17_41_14_299Z-debug.log

Log file:

$ cat /home/craig/.npm/_logs/2021-09-12T17_41_14_299Z-debug.log
0 verbose cli [
0 verbose cli   '/home/craig/.nvm/versions/node/v16.9.1/bin/node',
0 verbose cli   '/home/craig/.nvm/versions/node/v16.9.1/bin/npm',
0 verbose cli   'i',
0 verbose cli   'file:///home/craig/tmp/packages/b',
0 verbose cli   '-w',
0 verbose cli   'a'
0 verbose cli ]
1 info using [email protected]
2 info using [email protected]
...
30 timing idealTree:userRequests Completed in 1ms
31 silly idealTree buildDeps
32 silly placeDep ROOT [email protected] OK for: [email protected] want: file:/home/craig/tmp/packages/a
33 silly placeDep ROOT [email protected] OK for: [email protected] want: file:/home/craig/tmp/packages/b
34 timing idealTree:#root Completed in 5ms
35 silly placeDep packages/a b@ OK for: [email protected] want: file:packages/b
36 timing idealTree:packages/a Completed in 2ms
37 timing idealTree:node_modules/a Completed in 0ms
38 timing idealTree:node_modules/b Completed in 0ms
39 timing idealTree:packages/a/node_modules/b Completed in 0ms
40 timing idealTree:packages/a/packages/b Completed in 0ms
41 timing idealTree:packages/b Completed in 0ms
42 timing idealTree:buildDeps Completed in 8ms
43 timing idealTree:fixDepFlags Completed in 0ms
44 timing idealTree Completed in 26ms
45 timing reify:loadTrees Completed in 26ms
46 timing reify:diffTrees Completed in 0ms
47 silly reify moves {}
48 timing reify:retireShallow Completed in 0ms
49 timing reify:createSparse Completed in 0ms
50 timing reify:loadBundles Completed in 0ms
51 silly audit bulk request { a: [ '1.0.0' ] }
52 timing reifyNode:node_modules/a Completed in 18ms
53 timing reifyNode:packages/a/node_modules/b Completed in 18ms
...
64 timing command:install Completed in 75ms
65 verbose stack TypeError: Cannot set properties of null (setting 'dev')
65 verbose stack     at calcDepFlagsStep (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/calc-dep-flags.js:33:21)
65 verbose stack     at visit (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/calc-dep-flags.js:12:20)
65 verbose stack     at visitNode (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/treeverse/lib/depth-descent.js:57:25)
65 verbose stack     at next (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/treeverse/lib/depth-descent.js:44:19)
65 verbose stack     at depth (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/treeverse/lib/depth-descent.js:82:10)
65 verbose stack     at depth (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/treeverse/lib/depth.js:27:12)
65 verbose stack     at calcDepFlags (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/calc-dep-flags.js:10:15)
65 verbose stack     at Arborist.[copyIdealToActual] (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/reify.js:1299:7)
65 verbose stack     at Arborist.reify (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/reify.js:151:35)
65 verbose stack     at async Install.install (/home/craig/.nvm/versions/node/v16.9.1/lib/node_modules/npm/lib/install.js:150:5)
66 verbose cwd /home/craig/tmp
67 verbose Linux 5.4.0-48-generic
68 verbose argv "/home/craig/.nvm/versions/node/v16.9.1/bin/node" "/home/craig/.nvm/versions/node/v16.9.1/bin/npm" "i" "file:///home/craig/tmp/packages/b" "-w" "a"
69 verbose node v16.9.1
70 verbose npm  v7.21.1
71 error Cannot set properties of null (setting 'dev')
72 verbose exit 1

This also fails

$ npm i ./packages/b -w a
npm ERR! Cannot set properties of null (setting 'dev')
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/craig/.npm/_logs/2021-09-12T17_52_02_553Z-debug.log

Not being able to use already used package names should not be called a bug - I guess - but not giving an error message 'name taken' can be very confusing.

Not being able to install as a file? Is that a bug, a limitation, or a design choice feature?

Environment

@craigphicks craigphicks added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Sep 12, 2021
@beaucollins
Copy link

beaucollins commented Sep 23, 2021

We had this issue, or at least the same error output of Cannot set properties of null (setting 'dev') . I jumped into the calcDepFlagsStep source code and had it dump the node when its .target was null:

   // for links, map their hierarchy appropriately
   if (node.isLink) {
+    if (node.target == null) {
+      throw new Error('Node target is null: ' + JSON.stringify(node, null, ' '));
+    }
     node.target.dev = node.dev
     node.target.optional = node.optional
     node.target.devOptional = node.devOptional
     node.target.peer = node.peer
     return calcDepFlagsStep(node.target)
   }

It ended up being another of our own file-referenced packages file:../some/path and once we added it to our "workspaces":[] definition it was fixed.

In our particular case we were running:

npm ci --workspace a

Where a had a file reference dependency to a non-workspace package.

@craigphicks
Copy link
Author

craigphicks commented Sep 28, 2021

A working solution is use the exact version number of the "internal" package. That will override the "external package".

npm i [email protected] -w a

works to link the internal package, instead of

npm i b -w a

which installs the external package instead.

Yarn works similarly - it requires the exact version number.

So I'll close this issue and make a note on #2035

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

2 participants