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

Update dependencies and drop Node.js 8.x / stylelint < 13.0.0 support #123

Merged
merged 1 commit into from
Jan 22, 2020
Merged

Conversation

XhmikosR
Copy link
Contributor

  • eslint 6.8.0
  • sinon 8.0.4
  • stylelint 13.0.0
  • tape 4.13.0

@olegskl before merging this, maybe you should reconsider the stylelint peerDependency version? I'm a little lost, because stylelint 13.0.0 might work with Node.js 8.x but the engines version is now 10.x.

Also, I don't know if the old versions work with newer Node.js versions, so maybe this is a good chance to limit/change the peerDependency version?

Do note that for example, stylelint-config-recommended is using "stylelint": ">=10.1.0".

@XhmikosR
Copy link
Contributor Author

See also #124 in case you want to make 10.12.0 the minimum and drop the mkdirp dependency. I haven't tested it but it should work. Not sure if I'll have the time to work on this too, just letting you know :)

@XhmikosR XhmikosR mentioned this pull request Jan 12, 2020
@XhmikosR XhmikosR changed the title Update dependencies and remove Node.js 8.x support Update dependencies and drop Node.js 8.x support Jan 18, 2020
@XhmikosR XhmikosR requested a review from olegskl January 18, 2020 06:44
@olegskl
Copy link
Owner

olegskl commented Jan 22, 2020

I suppose we should release the new major version to match stylelint 13 and drop support for Node 8 as they have done. Then we can just change the stylelint peer dependency to ^13.0.0 to avoid compatibility issues. What do you think?

@XhmikosR
Copy link
Contributor Author

Technically gulp-stylelint should work with stylelint 10.1.0, though.

I don't know all this peerDependencies are hurting my brain :P I feel like we are overcomplicating things.

BTW in stylelint 13.0.0 there's no engines property set.

@XhmikosR XhmikosR changed the title Update dependencies and drop Node.js 8.x support Update dependencies and drop Node.js 8.x / stylelint < 13.0.0 support Jan 22, 2020
@XhmikosR
Copy link
Contributor Author

Patch updated. See also #125

* eslint 6.8.0
* sinon 8.1.1
* stylelint 13.0.0
* tape 4.13.0
@olegskl
Copy link
Owner

olegskl commented Jan 22, 2020

I remember that we had a long discussion (#35) about this, and the conclusion was to use peerDependencies 🤷‍♂

Copy link
Owner

@olegskl olegskl left a comment

Choose a reason for hiding this comment

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

👍

@XhmikosR
Copy link
Contributor Author

I'm not against peerDependencies. But since we only test the latest version, I don't know if an older one will break personally.

It should be possible to test the other versions too BTW using npm aliases.

Anyway, about this PR, if gulp-stylelint works with stylelint 13.0.0, we could just add v13.0.0 like I had in my initial patch

@XhmikosR
Copy link
Contributor Author

...and keep Node.js 8.x in package.json.

@olegskl
Copy link
Owner

olegskl commented Jan 22, 2020

Anyway, about this PR, if gulp-stylelint works with stylelint 13.0.0, we could just add v13.0.0 like I had in my initial patch and keep Node.js 8.x in package.json.

But then we can't merge #124 because it needs Node.js 10.x

I was planning to release these changes as gulp-stylelint@13.0.0 which requires stylelint@13.0.0. Currently we're at v11.0.0, so there will be a version skip. In case anyone needs Node 8.x with Stylelint 13 (which I doubt) we can cheat by releasing gulp-stylelint@12.

@XhmikosR
Copy link
Contributor Author

In that case I say just do all major changes in one batch. Merge this as is, then #124 and release a new major version.

@olegskl olegskl merged commit b3f155f into olegskl:master Jan 22, 2020
@XhmikosR XhmikosR deleted the deps branch January 22, 2020 18:42
@olegskl
Copy link
Owner

olegskl commented Jan 22, 2020

Released as v13.0.0. Thanks! 🙇

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

Successfully merging this pull request may close these issues.

2 participants