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

A note about upgrading dependencies from version 9 version 10 #2882

Closed
joshgoebel opened this issue Nov 19, 2020 · 11 comments
Closed

A note about upgrading dependencies from version 9 version 10 #2882

joshgoebel opened this issue Nov 19, 2020 · 11 comments
Labels
big picture Policy or high level discussion help welcome Could use help from community

Comments

@joshgoebel
Copy link
Member

joshgoebel commented Nov 19, 2020

If your library (or a dependency) depends on v9 and you're trying to upgrade to v10

Let's say you want to help upgrade an open source package that depends on Highlight.js v9. You'd like to help upgrade it to v10 because it's a dependency of your project but the maintainer seems busy, or no longer around - but you know JavaScript and you have some time to work on a PR. And of course even if the fix is easy if the author is no longer around getting that fix on NPM might require republishing the library or some such, but if so that'd just be what had to be done. Best case is the author is around (just busy) and could merge a PR if you could submit one.

So this can be SUPER easy or it can be insanely difficult - all depends on the library. lowlight is a popular dependent of Highlight.js. In prior versions (v9) they pretty much maintained their own fork of the library - because that's what they had to do to hook into the internals they needed. Not a trivial upgrade scenario.

Yet several of the libraries I've see at a glance just call our public API (highlight, highlightAuto, etc)... Our public API between v9 and v10 was remarkably stable.

It's possible some of these libraries might actually "just work" with ONLY a version bump:

# fetch the library
git clone older-library
cd older-library
# install the latest v10 version of Highlight.js
npm install highlight.js@latest 
# run tests maybe?
git commit
# make PR request

Obviously knowing a little about NPM and JavaScript will go a long ways if you wanted to help out here. If anyone has any specific questions about the process of upgrading or any of the breaking changes mentioned below, I'm happy to help as I can. I just don't have time to update all the possible dependents because NPM says that's almost 3000 packages. :-)

The biggest thing is we no longer support IE11 on the client-side... but if you're only using our library on the server-side that wouldn't matter at all.

Reference:

@arpowers
Copy link

arpowers commented Nov 20, 2020

FYI it's bad practice to add a runtime warning and even more so in a patch release.

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 20, 2020

✋🏻 I think I'd generally agree, though it's not "just" a patch release. It's the last release in the series, EOL announcement, etc.

FYI it's massive bad practice to do this in a patch.

What would you say is the best practice for something like this? We wanted everyone to see it and it's possible that some package maintainers pinned the minor version... so issuing a new minor release might not have had any effect for those users.

And just to be clear are you referring to the post-install or the console.log or the prior console.warn? (FYI: it's a console.log now, not a warning)

@arpowers
Copy link

arpowers commented Nov 20, 2020

You can add the warning during yarn install or npm install ...

https://docs.npmjs.com/cli/v6/commands/npm-deprecate

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 20, 2020

Yes, we also did those things also (though I was going to allow a small time window before deprecating 9.18.5)... though I don't have a strong reason - I'm just playing this by ear. I've also been told yarn hides post-install output.

Technically v9 was "theoretically" EOLed (made obsolete) back in April with the release of 10 and our official dropping of support for IE11 (the best argument not to switch to v10). But I'm not sure everyone got that message (I'd say the fact that we're having these discussions now shows not) - so hence our wanting to do it now very loudly so that everyone possible knows and we aren't still talking about v9 in 2021.

We can't of course stop anyone from continuing to use it, but we can make it clear we feel it's a bad idea. Re: Deprecate. So many people I've spoken with pretty much ignore it completely, so it didn't seem a great "primary" choice. I wholly agree warn vs log was a blunder on my part. :-)

Out of curiosity is the console.log causing you any particular problem with your setup or you just wanted to add a not on best practices?

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 20, 2020

You can add the warning during yarn install or npm install ...

Also we've had several people tell us "please don't do that"... so it seems not everyone is on the same page on this. :) Unless you specially mean ONLY deprecate when you say "warning during install" - but back to my point about people just ignoring those...

So far I have a sneaking suspicion that while we could have done this better there was no perfect way to go about it.

You've encouraged me to go ahead and run deprecate on 9.18.5 also.

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 20, 2020

#2881

If you have any specific thoughts (or articles) on how to clearly communicate EOL to users of NPM packages and wanted to share on that thread we're very willing to learn for when the 10 -> 11 transition comes around. I'm not sure that's even really a thing in the NPM/Node.js culture though. deprecate seems not much used and the API buggy in my experience. I look on NPM and find packages (including our own) with versions going back years and years - some with serious (and not so serious security issues). We still had our 1.0.0 on there until just the other day.

Yesterday I deprecated everything except for our newest releases (that we actively support here via issues, etc).

And while we could perhaps issue CVEs (or just advisories) against old versions - that's a bit more nebulous (correct me if I'm wrong) when we're confident there are issues (or strong potential issues) but you don't know exactly what they are.

Plus my feeling is saying "upgrade now things are going to get bad later" is better than proving it by dropping a bunch of security advisories on people at a later date - with no fixes forthcoming. IE, if we have a chance to provide some warning - that seems better. Perhaps you disagree?

@arpowers
Copy link

arpowers commented Nov 21, 2020

You have to understand your users. There is no benefit to upgrade in most cases; for example, markdown-it includes your dep and is now throwing the error for me. I honestly don't care about what version of highlight.js is being used

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 21, 2020

To be clear I don’t know what you mean “throwing the error“. 9.18.5 does not throw or raise any new errors. Neither did .4. It did log to STDERR though - which was a mistake and was corrected with .5.

If you’re stuck on .4 as a few people have mentioned removing the markdown-it dependency and then re-adding it will likely fix it.

@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 21, 2020

You have to understand your users. There is no benefit to upgrade in most cases;

Even if (just for the sake of discussion) I agreed with your second statement... you have to realize the opposite is also true - there are absolutely some [serious] benefits in some use cases to upgrade. In such cases there are also very real downsides and potential costs for not upgrading. Some users care very, very much.

The the last 9.18 update should not have broken anyones projects. The fact that it did (via writing to STDERR) has been resolved (the same day) and 9.18.5 should be a drop-in replacement for 9.18.3. If you're on 9.18.5 and seeing a specific regression compared to 9.18.x you'd need to provide additional details - and I possibly can help.

There is no benefit to upgrade in most cases

If you changed this to "there is little benefit for some users" I might agree - but when pushing an update like this sadly there is no way to easily separate who is who with such specificity (not that I know of at least)!

Anyways like I'm pretty sure I already said: perhaps we did indeed get this a bit wrong and not handle it optimally... but we're doing our best and we're just going to keep moving forward and learning as we go.

@joshgoebel
Copy link
Member Author

Closing this in favor of it's mention at #2877.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
big picture Policy or high level discussion help welcome Could use help from community
Projects
None yet
Development

No branches or pull requests

3 participants
@joshgoebel @arpowers and others