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

Eslint v8.1 breaks docker deployment #2407

Closed
manekenpix opened this issue Oct 28, 2021 · 7 comments
Closed

Eslint v8.1 breaks docker deployment #2407

manekenpix opened this issue Oct 28, 2021 · 7 comments
Labels
area: docker dependencies Pull requests that update a dependency file type: bug Something isn't working
Projects

Comments

@manekenpix
Copy link
Member

manekenpix commented Oct 28, 2021

After merging #2386, our staging server broke. The issue is that @typescript-eslint/eslint-plugin and @typescript-eslint/parser don't support eslint v8.1 yet (here's the issue in their repo).

Since we don't use devDependencies in production, this bug should never happen, BUT a closer look to what npm does when installing dependencies in production reveals another issue.

Command: `npm install --only=production --verbose --no-package-lock --ignore-scripts`

...
#9 11.57 npm http fetch GET 200 https://registry.npmjs.org/@babel%2fpreset-react 62ms (cache miss)
#9 12.22 npm http fetch GET 200 https://registry.npmjs.org/@babel%2fpreset-typescript 640ms (cache miss)
#9 12.28 npm http fetch GET 200 https://registry.npmjs.org/@types%2fjest 62ms (cache miss)
#9 12.50 npm http fetch GET 200 https://registry.npmjs.org/@typescript-eslint%2feslint-plugin 211ms (cache miss)
#9 12.74 npm http fetch GET 200 https://registry.npmjs.org/@typescript-eslint%2fparser 217ms (cache miss)
#9 12.83 npm http fetch GET 200 https://registry.npmjs.org/eslint 74ms (cache miss)
...

It seems npm does some sort of prefetching (including devDependencies), and it's what's causing this bug.

The 2 possible solutions are:

@manekenpix manekenpix added type: bug Something isn't working area: docker Priority: High dependencies Pull requests that update a dependency file labels Oct 28, 2021
@manekenpix manekenpix added this to To do in CI/CD via automation Oct 28, 2021
@manekenpix manekenpix pinned this issue Oct 28, 2021
@humphd
Copy link
Contributor

humphd commented Oct 28, 2021

cc @BeAmazedVariable.

Two thoughts. First, we should probably revert #2386 based on the upstream issue, which doesn't sound like support for 8.1 is coming soon. @BeAmazedVariable can you do the following in a new PR for me?

git checkout master
git pull upstream master
git checkout -b issue-2407
git revert 3d7a3336d4bf89b6174b81c0999c29f411490ffc

And then make a new PR and submit?

Second, it looks to me like npm install has changed its syntax from --only=production to --production:

With the --production flag (or when the NODE_ENV environment variable is set to production), npm will not install modules listed in devDependencies. To install all modules listed in both dependencies and devDependencies when NODE_ENV environment variable is set to production, you can use --production=false.

NOTE: The --production flag has no particular meaning when adding a dependency to a project.

I wonder if changing how we do the install to use --production would solve the prefetching of devDependencies?

@tpmai22
Copy link
Contributor

tpmai22 commented Oct 28, 2021

@humphd I would love to jump on it !

@tpmai22
Copy link
Contributor

tpmai22 commented Oct 28, 2021

I wonder if changing how we do the install to use --production would solve the prefetching of devDependencies?

when I install all the dependencies above I use npm install eslint@latest or npm install eslint@8.1.0. I will make another attempt on using --production and see how it go.

@humphd
Copy link
Contributor

humphd commented Oct 28, 2021

@BeAmazedVariable you don't need to do that, just use the revert I mentioned. That was for @manekenpix

@manekenpix
Copy link
Member Author

@humphd

Second, it looks to me like npm install has changed its syntax from --only=production to --production:

Yesterday I tried --production (I was trying to fix the bug) and I got the same result. Can you try to make sure that it's not me?

@humphd
Copy link
Contributor

humphd commented Oct 28, 2021

I haven't had time to dig into this fully yet, but I wonder if we're hitting an npm bug, maybe https://npm.community/t/npm-install-only-prod-verbose-please-do-not-pre-fetch-devdependencies/4732? I need to see if they ever fixed this. Sounds like maybe they didn't.

@manekenpix
Copy link
Member Author

Closed via #2408

CI/CD automation moved this from To do to Done Oct 31, 2021
@humphd humphd unpinned this issue Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docker dependencies Pull requests that update a dependency file type: bug Something isn't working
Projects
No open projects
CI/CD
  
Done
Development

No branches or pull requests

3 participants