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

chore: fix Husky #4041

Merged
merged 1 commit into from
Jan 14, 2022
Merged

chore: fix Husky #4041

merged 1 commit into from
Jan 14, 2022

Conversation

ehmicky
Copy link
Contributor

@ehmicky ehmicky commented Jan 14, 2022

See netlify/js-client#659

This fixes how we use Husky with its latest version.


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

@ehmicky ehmicky added the type: chore work needed to keep the product and development running smoothly label Jan 14, 2022
@ehmicky ehmicky self-assigned this Jan 14, 2022
erezrokah
erezrokah previously approved these changes Jan 14, 2022
@github-actions
Copy link

📊 Benchmark results

Comparing with ed62e19

Package size: 448 MB

⬆️ 20.88% increase vs. ed62e19

^                                                                                                  448 MB 
│                                                                                                   ┌──┐  
│                                                                                                   |▒▒|  
│                                                                                                   |▒▒|  
│  356 MB  356 MB  356 MB  356 MB  356 MB  356 MB  356 MB  356 MB  356 MB  356 MB  356 MB  356 MB   |▒▒|  
│ ──┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────┌──┐────|▒▒|──
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

@@ -20,7 +20,7 @@ jobs:
cache-dependency-path: 'npm-shrinkwrap.json'
check-latest: true
- name: Install dependencies
run: npm install --production --no-audit
run: npm ci --no-audit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

husky is now an indirect dependency (through @netlify/eslint-config-node) instead of being direct.

Binaries of indirect dependencies are symlinked by npm to the top-level node_modules/.bin/ thanks to deduping. However, this seems to only work without the --production flag.

Also, this is more consistent with the other GitHub actions which all use npm ci --no-audit. Please note this is only for the GitHub action which computes the package size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this give the wrong result now, though? It now includes devDependencies :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ehmicky How about doing npm prune --prod after npm ci? It's not ideal but it should be close to what it was before :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #4066

@ehmicky ehmicky added the automerge Add to Kodiak auto merge queue label Jan 14, 2022
@kodiakhq kodiakhq bot merged commit 8ed72d5 into main Jan 14, 2022
@kodiakhq kodiakhq bot deleted the chore/fix-husky branch January 14, 2022 17:44
@XhmikosR
Copy link
Contributor

@ehmicky this breaks production installs which probably is the same reason 8.9.0 release failed :/

I tried to install the latest main version from GitHub and it fails with the same error as the release action:

C:\Users\xmr\Desktop>npm i -g github:netlify/cli#main
npm WARN deprecated source-map-url@0.4.1: See https://github.com/lydell/source-map-url#deprecated
npm WARN deprecated flatten@1.0.3: flatten is deprecated in favor of utility frameworks such as lodash.
npm WARN deprecated urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated rollup-plugin-inject@3.0.2: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-inject.
npm WARN deprecated source-map-resolve@0.5.3: See https://github.com/lydell/source-map-resolve#deprecated
npm WARN deprecated resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
npm WARN deprecated statsd-client@0.4.7: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
npm WARN deprecated node-pre-gyp@0.13.0: Please upgrade to @mapbox/node-pre-gyp: the non-scoped node-pre-gyp package is deprecated and only the @mapbox scoped package will recieve updates in the future
npm ERR! code 1
npm ERR! git dep preparation failed
npm ERR! command C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js install --force --cache=C:\Users\xmr\AppData\Local\npm-cache --prefer-offline=false --prefer-online=false --offline=false --no-progress --no-save --no-audit --include=dev --include=peer --include=optional --no-package-lock-only --no-dry-run
npm ERR! npm WARN using --force Recommended protections disabled.
npm ERR! npm ERR! code 1
npm ERR! npm ERR! path C:\Users\xmr\AppData\Local\npm-cache\_cacache\tmp\git-cloneCg974T
npm ERR! npm ERR! command failed
npm ERR! npm ERR! command C:\WINDOWS\system32\cmd.exe /d /s /c husky install node_modules/@netlify/eslint-config-node/.husky/
npm ERR! npm ERR! 'husky' is not recognized as an internal or external command,
npm ERR! npm ERR! operable program or batch file.
npm ERR!
npm ERR! npm ERR! A complete log of this run can be found in:
npm ERR! npm ERR!     C:\Users\xmr\AppData\Local\npm-cache\_logs\2022-01-15T07_29_00_041Z-debug.log

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\xmr\AppData\Local\npm-cache\_logs\2022-01-15T07_29_00_267Z-debug.log

Maybe it's time to reconsider reverting the husky update after all? It's been just a pain in the butt :/

@ehmicky
Copy link
Contributor Author

ehmicky commented Jan 15, 2022

Thanks for finding this out @XhmikosR!
I have created #4046 to fix this. Let's try to first just add husky as a devDependency, before reverting, as I believe this should fix this problem.

@ehmicky
Copy link
Contributor Author

ehmicky commented Jan 15, 2022

The PR is merged now, which should fix the problem. 👍
Thanks again for reporting it!

@XhmikosR
Copy link
Contributor

I don't think that's the reason, unfortunately :/

In production you do npm prune --production which probably conflicts.

@XhmikosR
Copy link
Contributor

You could probably try npm pack to see this failing.

@ehmicky
Copy link
Contributor Author

ehmicky commented Jan 15, 2022

Great catch again!
Adding husky solved the problem in the other repositories, but we run some prepack and postpack npm scripts with Netlify CLI. prepack runs before prepare, so husky is missing since it is a devDependencies.

So, at the moment, npm publish will fail with an husky: not found error. Note: this bug should not impact any users yet, since we haven't released anything. However, it prevents us from publishing new releases.

I am wondering whether we could just remove those prepack and postpack scripts. 🤔
npm publish already exclude devDependencies, so there is no need to prune them.
I tried running npm pack --dry before and after removing prepack and postpack, and the same bundle seems to be produced, with the same files.

PR at #4049

@XhmikosR
Copy link
Contributor

You could probably remove them, but there must be some reason they exist.

Could be they were needed in the past or something. Or they were added as a precaution measure so that the lock file is as small as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants