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(deps): update dependencies #1003

Merged
merged 8 commits into from
Oct 4, 2021
Merged

chore(deps): update dependencies #1003

merged 8 commits into from
Oct 4, 2021

Conversation

iiroj
Copy link
Member

@iiroj iiroj commented Aug 6, 2021

This PR updates all top-level dependencies, expect for [email protected] which is now ESM-only and thus won't work in lint-staged until it is also converted to ESM.

What do you think, @okonet?

@iiroj iiroj requested a review from okonet August 6, 2021 05:45
@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #1003 (29d4356) into master (3885af8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1003   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           19        20    +1     
  Lines          641       646    +5     
  Branches       146       146           
=========================================
+ Hits           641       646    +5     
Impacted Files Coverage Δ
lib/figures.js 100.00% <100.00%> (ø)
lib/messages.js 100.00% <100.00%> (ø)
lib/resolveTaskFn.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3885af8...29d4356. Read the comment docs.

@iiroj
Copy link
Member Author

iiroj commented Sep 26, 2021

I rebased this and updated again, but it seems even more of @sindresorhus's packages have migrated to ESM and thus can no longer be updated.

I was able to migrate lint-staged itself to ESM pretty easily, but the jest tests seem a bit more complicated. Some tests seems to fail because they "cannot use import statement outside a module", so it's probably related to Babel.

What do you think, @okonet?

@okonet
Copy link
Collaborator

okonet commented Sep 27, 2021

Sorry but I don't have an opinion on that one. I'm not sure why we need to migrate to ESM since lint-staged can't be used as a dependency.

@iiroj
Copy link
Member Author

iiroj commented Sep 27, 2021

To keep dependencies up-to-date, we need to either stop updating ESM deps, replace them, or convert to ESM ourselves.

Lint-staged does expose a Node.js API:

import lintStaged from "lint-staged";

Personally I'm fine with anything, although if we stay at CommonJS, it might make sense to inline some simpler dependencies into the project directly.

@okonet
Copy link
Collaborator

okonet commented Sep 28, 2021

Hmm, yeah I forgot about providing the Node API :)

I'm fine with everything too but I see you point of being up to date i.e. for security reasons. Can we ping someone from Jest / Babel to help us with this?

@iiroj
Copy link
Member Author

iiroj commented Sep 28, 2021

@okonet let's release all the updates we can do without changing anything, and then revisit once we know which dependencies can no longer be updated, and how the Node.js version requirements would change if we were to go to ESM.

I need to rebase this branch.

@iiroj
Copy link
Member Author

iiroj commented Oct 2, 2021

Ping @okonet, I rebased this and added some optimizations to reduce dependency sizes. See these commits: 5240c26 4de4cda

@iiroj
Copy link
Member Author

iiroj commented Oct 2, 2021

Ping @okonet this renovation got a little out of hand, and I also updated all the GitHub CI Action runners, as well as replace yarn with npm. Since the new setup-node supports caching out-of-box, using npm should be fine for performance.

Thoughts?

@okonet
Copy link
Collaborator

okonet commented Oct 3, 2021

LGTM

@iiroj
Copy link
Member Author

iiroj commented Oct 3, 2021

@okonet let's merge this as feat so that we can skip the buggy 11.1.3 and 11.1.4 published into lint-staged@next from here: #1022

@iiroj iiroj merged commit 32c08d3 into master Oct 4, 2021
@iiroj iiroj deleted the update-deps branch October 4, 2021 10:56
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2021

🎉 This PR is included in version 11.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iiroj iiroj mentioned this pull request Oct 4, 2021
1 task
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2021

🎉 This PR is included in version 11.3.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

"debug": "^4.3.1",
"cli-truncate": "2.1.0",
"colorette": "^1.4.0",
"commander": "^8.2.0",
Copy link

Choose a reason for hiding this comment

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

This dependency upgrade bumps the minimum node.js version from 10 to 12. So maybe that should be considered as breaking change instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It for sure should have been! Thanks for pointing this out. cc @iiroj

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! Maybe a fix to downgrade? Or should we bump our version now that 16 is in LTS and 10 is getting old?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd bump and release as a major TBH

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured we could go all the way and convert lint-staged to ESM: #1038

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we already require Node.js 12.13, we just don't enforce it via the engines.node key:

https://github.com/okonet/lint-staged/blob/e035b80e39da355da57c02db6565b55271ab1afa/bin/lint-staged.js#L20

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

Successfully merging this pull request may close these issues.

3 participants