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.

Sign up for GitHub

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

Jump to bottom

tools: update-eslint.sh does not work with npm@7 #37560

Closed
lpinca opened this issue Mar 1, 2021 · 6 comments
Closed

tools: update-eslint.sh does not work with npm@7 #37560

lpinca opened this issue Mar 1, 2021 · 6 comments
Labels
npm Issues and PRs related to the npm client dependency or the npm registry. tools Issues and PRs related to the tools directory.

Comments

@lpinca
Copy link
Member

lpinca commented Mar 1, 2021 •
edited
Loading

  • Version: v15.10.0, npm version 7.5.3
  • Platform: Darwin imac.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64
  • Subsystem: tools

What steps will reproduce the bug?

Running the tools/update-eslint.sh with npm@7 fails with the following error:

npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.

How often does it reproduce? Is there a required condition?

Always. No required condition.

What is the expected behavior?

The script works without errors.

What do you see instead?

See above.

Additional information

Using the --legacy-peer-deps flag to install eslint-plugin-markdown does not help as it produces a new issue where the eslint-tmp/node_modules/eslint folder is removed.

I can only make it work with something like this

diff --git a/tools/update-eslint.sh b/tools/update-eslint.sh
index d0320ab8f8..69dab66a2c 100755
--- a/tools/update-eslint.sh
+++ b/tools/update-eslint.sh
@@ -8,20 +8,14 @@
 # $BASH_SOURCE[0] to determine directories to work in.
 
 cd "$( dirname "$0" )" || exit
-rm -rf node_modules/eslint
+rm -rf node_modules/eslint node_modules/eslint-plugin-markdown
 (
     mkdir eslint-tmp
     cd eslint-tmp || exit
     npm init --yes
 
     npm install --global-style --no-bin-links --production --no-package-lock eslint@latest
-
-    (
-        cd node_modules/eslint || exit
-
-        npm install --no-bin-links --production --no-package-lock eslint-plugin-markdown@latest
-    )
-
+    npm install --global-style --no-bin-links --production --no-package-lock eslint-plugin-markdown@latest
 
     # Use dmn to remove some unneeded files.
     npx [email protected] -f clean
@@ -31,4 +25,6 @@ rm -rf node_modules/eslint
 )
 
 mv eslint-tmp/node_modules/eslint node_modules/eslint
+mv eslint-tmp/node_modules/eslint-plugin-markdown node_modules/eslint-plugin-markdown
+
 rm -rf eslint-tmp/

but that means adding a new folder and some dependencies are probably not deduplicated.
@lpinca lpinca added npm Issues and PRs related to the npm client dependency or the npm registry. tools Issues and PRs related to the tools directory. labels Mar 1, 2021
@lpinca
Copy link
Member Author

lpinca commented Mar 1, 2021

cc: @nodejs/npm

@ruyadorno

This comment has been minimized.

Sign in to view
@ruyadorno
Copy link
Member

ruyadorno commented Mar 1, 2021 •
edited
Loading

@lpinca I'm just trying to reproduce it here and now I realize the current script is cding into a node_modules folder? That's not a supported use case as far as I'm aware.

What is the end goal here? I'm thinking that your proposed change (from the attached patch diff) might actually be a much better way to handle it - but again I don't have the full context here.

@lpinca
Copy link
Member Author

lpinca commented Mar 1, 2021 •
edited
Loading

Yes current script is adding eslint-plugin-markdown as a dependency of eslint after installing it. I don't know what are the motivations to do that. Perhaps moving only one directory and reduce dependencies?

Refs: #13895

cc: @Trott

I'm fine with the above patch if there is consensus.

@aduh95
Copy link
Contributor

aduh95 commented Mar 1, 2021

How does the git diff looks with this patch? Do you see a bunch of added files or does it just move them?

@Trott
Copy link
Member

Trott commented Mar 2, 2021

IIRC, the reason we cd into the node_modules/eslint is so that eslint can have its own node_modules so that we can move the eslint directory to a different location later and still have everything work.

lpinca added a commit to lpinca/node that referenced this issue Mar 2, 2021
@lpinca
tools: make update-eslint.sh work with npm@7
8e71d35 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

Fixes: nodejs#37560
@lpinca lpinca mentioned this issue Mar 2, 2021
Closed
lpinca added a commit to lpinca/node that referenced this issue Mar 2, 2021
@lpinca
tools: make update-eslint.sh work with npm@7
d029772 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

Fixes: nodejs#37560
lpinca added a commit to lpinca/node that referenced this issue Mar 2, 2021
@lpinca
tools: make update-eslint.sh work with npm@7
151fbb7 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

Fixes: nodejs#37560
lpinca added a commit to lpinca/node that referenced this issue Mar 3, 2021
@lpinca
tools: make update-eslint.sh work with npm@7
708364d 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

Fixes: nodejs#37560
lpinca added a commit to lpinca/node that referenced this issue Mar 3, 2021
@lpinca
tools: make update-eslint.sh work with npm@7
a6ec31e 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

Fixes: nodejs#37560
@lpinca lpinca closed this as completed in aee3ef5 Mar 4, 2021
danielleadams pushed a commit that referenced this issue Mar 16, 2021
@lpinca @danielleadams
tools: make update-eslint.sh work with npm@7
fd7234c 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

PR-URL: #37566
Fixes: #37560
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this issue May 30, 2021
@lpinca @targos
tools: make update-eslint.sh work with npm@7
d13983e 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

PR-URL: #37566
Fixes: #37560
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
@lpinca @targos
tools: make update-eslint.sh work with npm@7
d7ed4ec 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

PR-URL: #37566
Fixes: #37560
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
@lpinca @targos
tools: make update-eslint.sh work with npm@7
71ad9af 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

PR-URL: #37566
Fixes: #37560
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this issue Jun 11, 2021
@lpinca @targos
tools: make update-eslint.sh work with npm@7
5de911e 
Install `eslint-plugin-markdown` at the same level of `eslint` without
cd'ing into `eslint` directory, otherwise the following error is raised:

```
npm ERR! code ERESOLVE
npm ERR! Cannot destructure property 'name' of 'node' as it is null.
```

PR-URL: #37566
Fixes: #37560
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
npm Issues and PRs related to the npm client dependency or the npm registry. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

tools: make update-eslint.sh work with npm@7 lpinca/node
4 participants
@ruyadorno @Trott @lpinca @aduh95