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

downgrade lint-staged #6611

Closed
wants to merge 1 commit into from
Closed

Conversation

Mouvedia
Copy link
Contributor

@Mouvedia Mouvedia commented Feb 3, 2023

context

see: 2290f55#r99179433

reason/motivation

I had an error while commiting (because of .?)

@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2023

🦋 Changeset detected

Latest commit: 2bb4686

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Mouvedia Mouvedia marked this pull request as draft February 3, 2023 14:08
@Mouvedia Mouvedia changed the title downgrade lint-staged / add engine-strict / emend engine.nodes downgrade lint-staged / emend engine.nodes Feb 3, 2023
@Mouvedia Mouvedia marked this pull request as ready for review February 3, 2023 14:10
@Mouvedia Mouvedia requested a review from jeddy3 February 3, 2023 14:16
package.json Outdated Show resolved Hide resolved
@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 4, 2023

…but engines.node downgrade may affect users. Is the latter necessary?

You are right to have that worry.
We are about to release a major release; I would like to have introduced that change before that.
Right now you can't really say why you have chosen ^14.13.1 over ^14.0.0.
You have three choices either :

  1. that specific version introduced a feature before the LTS that is necessary (but that only works for the bottom range (i.e. ^12.20.0)
  2. always pick the format major.0.0 format i.e. "^12.0.0 || ^14.0.0 || >=16.0.0"
  3. or use LTS because it's a good convention (what I am proposing)

It cannot be 1. in our case, so we should pick something sensible that can be justified.
i.e. either "^12.13.0 || ^14.15.0 || >=16.13.0" or "^12.0.0 || ^14.0.0 || >=16.0.0".

This is not urgent but it's a nice-to-have, that would permit to have the documentation expose the reason behind our choices.

@ybiquitous
Copy link
Member

Right now you can't really say why you have chosen ^14.13.1 over ^14.0.0.

The reason to chosen ^14.13.1 is to consider ESM support at that time. See #5303 (comment)

use LTS because it's a good convention (what I am proposing)

What are the benefits to adopt LTS? Adopting "major.0.0 format" seems better to me because it can support people or environments more widely, unless there is a special reason like ESM support.


In addition, we've dropped Node.js 12 support on the v15 branch, so there seems to be no benefit to downgrade lint-staged on the main branch. See:

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 6, 2023

See #5303 (comment)

I didn't know, my bad. Now it makes perfect sense; Ill remove the commit.
It actually was 1. in my list, I just didn't know the feature.

What are the benefits to adopt LTS?

You can read about LTS here.

In addition, we've dropped Node.js 12 support on the v15 branch…

This was meant for the last release before v15.

@Mouvedia Mouvedia changed the title downgrade lint-staged / emend engine.nodes downgrade lint-stagedengine.nodes Feb 6, 2023
@Mouvedia Mouvedia changed the title downgrade lint-stagedengine.nodes downgrade lint-staged Feb 6, 2023
@ybiquitous
Copy link
Member

I think the v15.0.0 release will be soon, so can we avoid this lint-staged downgrade? Perhaps, many conflicts may come in package-lock.json... 😓

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 6, 2023

I don't know. I was bothered so I PRd.
Is it urgent or important? It's not.
FYI I used Node.js 16 to regenerate the package-lock.json so that the version remained 2.

@jeddy3
Copy link
Member

jeddy3 commented Feb 6, 2023

I think the v15.0.0 release will be soon, so can we avoid this lint-staged downgrade?

Yes, let's close.

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 6, 2023

eslint-plugin-jest seems to be affected too:

const globalAliases = ((_context$settings$jes = context.settings.jest) === null || _context$settings$jes === void 0 ? void 0 : _context$settings$jes.globalAliases) ?? {};

gives a SyntaxError.

@ybiquitous
Copy link
Member

@Mouvedia Which version of Node.js do you use?

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 7, 2023

@Mouvedia Which version of Node.js do you use?

It depends on the tab or if it's a terminal in my IDE.
I use nvm so I switch often. On this machine: 6, 8, 10, 12, 14 ,16.

@ybiquitous
Copy link
Member

I use nvm so I switch often

Can you put a .nvmrc file in the project root to satisfy the following engines.node? If doing so, can we mitigate problems due to different Node.js versions? We can also accept adding .nvmrc to .gitignore.

"node": "^14.13.1 || >=16.0.0"

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 7, 2023

We can also accept adding .nvmrc to .gitignore.

👍

Yes, let's close.

Now that it has been merged in main, we have to.

@Mouvedia Mouvedia closed this Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants