search

SeanSobey / ChartjsNodeCanvas

A node renderer for Chart.js using canvas.
MIT License
230 stars 79 forks source link

Version 3.0.7 is a breaking change! It should upraise MAJOR version. #60

Closed padys closed 3 years ago

padys commented 3 years ago

Main class CanvasRenderService in 3.0.7 is replaced by ChartJSNodeCanvas. It is a real breaking change and MAJOR version of package should be upraised to 4.x.x.

SeanSobey commented 3 years ago

Apologies to anyone for any issues/inconvenience caused by this. I am not quite sure where it went wrong, but the plan was for the 2.x.x to 3.x.x major bump to be this breaking change (i.e CanvasRenderService to ChartJSNodeCanvas) but somehow up until 3.0.6 the old API was released.

I think it was the included migration to GitHub actions publish at the same time. Lesson learned, don't do API updates and CI pipeline changes at the same time.

Based on the download stats, it seems most people saw the 2.6.1 -> 3.0.6 -> 3.1.0 upgrade. So while this was definitely a bad situation, I don't think bumping the version to 4.x.x will help? I am not very familiar with package version management though so maybe I am missing/misunderstanding something?

hardillb commented 3 years ago

I'm sorry this doesn't work. If you introduce a breaking change you MUST bump the major version number or npm will just force the broken code on anybody pushed a package with 3.0.x dependency. You should also remove all the 3.0.6+ releases.

Moving to 3.1.x doesn't work as that is also meant to be totally backward compatible with all 3.0.x releases.

Please take the time to read about how semantic versioning works https://docs.npmjs.com/about-semantic-versioning

SeanSobey commented 3 years ago

Again apologies for the inconvenience caused by this update.

So there was a major bump for this change, which was 2.x.x to 3.x.x, and included notes in the changelog and readme. I assume most people would have seen the breaking change when the code updates were actually pushed, which in my understanding is where I went wrong.

But still people would have had to update a major version for this change. The update from 2.4.1 to 3.0.6 should have, but did not, include this but then saw it in 3.1.0. I should have bumped that a major version to correct the mistake, but since I only saw this issue after the fact and that version is already out, how does doing this now help?

hardillb commented 3 years ago

The correct response is to roll back the changes on the whole 3.x branch to the old API and publish 3.1.1 and 3.0.10 with the old API and 4.0.0 with the new API.

This means that anything build against pre 3.0.6 will work with the default npm upgrade paths and anybody wanting the new API should target 4.0.x.

There is no good solution to this, but this is the one that should leave the most code working.

SeanSobey commented 3 years ago

With the amount of time between 3.0.6 and 3.1.0 and now will this not just cause the same thing in reverse again?

hardillb commented 3 years ago

But 3.1 HAS to be backward compatible with 3.0.0 according to the versioning system

No matter what you do you're going to break people, but the best option is to do what the versioning scheme says it should do, then there is some logic behind things

hardillb commented 3 years ago

The only other solution I can think is if 3.1 can support both signatures for the constructor, this would be the best compromise as it should break nobody but still allow a clean move to 4.0.x

SeanSobey commented 3 years ago

I had a look through some dependants versions and along with the npm stats I think your suggestion will limit the problem to the least number of people, as well as any future installs of version 3.x.x.

I like the idea of both signatures for this stopgap release, I will investigate that now and try see what I can do, otherwise deploy a rollback version and new version 4.0.0