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

Drop dependency on deprecated gulp-util #100

Merged
merged 1 commit into from Jan 3, 2018

Conversation

demurgos
Copy link
Contributor

Closes #99

@stephenlacy
Copy link

stephenlacy commented Dec 28, 2017

Just checking, did you want to include the lock files in the pr?

@OverZealous
Copy link
Owner

@demurgos Thank you for handling this PR!

I'll have to look deeper into the lock files to see how I feel about them (though unless it changes anything else drastically, I'm fine with it in principle).

I'll most likely accept this PR, but I think I'll have to make this a breaking change release, so it's something I'll want to deal with when I have more time, hopefully over the weekend.

@demurgos
Copy link
Contributor Author

@stevelacy
Yes, the lock files should be there. They do not affect consumers of library but help developers. They provide a known dependency that works and ensure reproducibility for other developers of the library and CI. It is important to commit them. I did not have any issues with your library but over the last few days I opened many PRs to remove gulp-util from other libraries and the lack of lock files and irregular maintenance caused many issues (especially because of transitive dependencies).

From a semver point of view, this PR should be a patch version. You can still do a major bump, but it will then require consumers of your library to manually update their dependencies instead of immediately benefiting from the patch.

@demurgos
Copy link
Contributor Author

demurgos commented Jan 1, 2018

@stevelacy
I removed the lock file from my PR. They solve a different problem and this PR should focus of fixing gulp-util.

@OverZealous
Do you want some help to handle the update you planned?

@OverZealous
Copy link
Owner

It's not something I need help with, I just need to make sure I understand the consequences. I haven't had time to look into it yet. I'll try to get to it this week.

@OverZealous OverZealous mentioned this pull request Jan 3, 2018
@OverZealous
Copy link
Owner

@demurgos Why did you add the package-lock to .gitignore? I believe it's considered correct & standard practice to include the locks in libraries, for consistent behavior.

I was thinking about removing yarn.lock from the repo, since I don't use yarn (npm caught up with the package-lock for the feature I cared about, and yarn broke a bunch of things for me when it came out). So I actually don't mind that it was removed.

I don't want to simply replace your efforts, so if you could do the following I'll get this merged in an deployed with maybe a minor version bump, since I can't see any major side effects:

  • Undo the changes to .gitignore

Actually, I thought there was more, it's just that 😆. You can include the package-lock.json or I'll update it locally before I bump the version.

@demurgos
Copy link
Contributor Author

demurgos commented Jan 3, 2018

Sure, I'll remove the changes relative to the lock files. I shouldn't have touched them. Even if I personally use lockfiles everywhere, it is not always obvious for libraries (the counter argument is that your CI is no longer tested against the latest version of your dependencies).

You may still want to keep the yarn file around, it is used by Travis.

@OverZealous
Copy link
Owner

OverZealous commented Jan 3, 2018

Yeah, I'm with you (it wasn't me who questioned the lockfile above). I prefer to include lockfiles in my libraries for the reasons you mentioned. If you simply want to include both lockfiles, I'm 100% on board with that, too. :-)

And thanks again!

@demurgos
Copy link
Contributor Author

demurgos commented Jan 3, 2018

Okay, I'll push the fix in a few minutes

@OverZealous
Copy link
Owner

Awesome, thanks! I'll get the version bump pushed up and out in a minute. I think you are right that a patch is safe enough for this—I've just had too many things break due to supposedly minor updates in the past, so I'm a bit gun shy on my own libraries.

@OverZealous OverZealous merged commit 209e46c into OverZealous:master Jan 3, 2018
@OverZealous
Copy link
Owner

Published as v2.2.1.

@demurgos
Copy link
Contributor Author

demurgos commented Jan 3, 2018

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.

None yet

3 participants