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

feat(build)!: pretty much complete overhaul #88

Merged
merged 5 commits into from
May 17, 2020
Merged

feat(build)!: pretty much complete overhaul #88

merged 5 commits into from
May 17, 2020

Conversation

Thomaash
Copy link
Member

@Thomaash Thomaash commented May 16, 2020

BREAKING CHANGE: This is still mostly a drop in replacement for the old 4.21 Vis.js but feature wise there were some breaking changes in the past 7 months.

This brings the reexported versions inline with our other packages.

All the issues that have accumulated over the past 7 months (since the last release) are resolved here.

BREAKING CHANGE: This is no longer a drop in replacement for the old
4.21 Vis.js.

This brings this project inline with our other packages (except for TS
support, there is none).

I also removed the notice about this being a drop in replacement for the
old Vis and broke backwards compatibility by simply reexporting up to
date packages. I don't have the time to manage backwards compatibility
here and I don't even think it's worth it at all.

All the issues that have accumulated over the past 7 months (since the
last release) are resolved here.
@Thomaash Thomaash requested a review from a team May 16, 2020 17:41
@yotamberk
Copy link
Member

Awesome work! I think it might be worth changing the commit from "feat" to the tag that changes the major version since the great work done here is a breaking change

yotamberk
yotamberk previously approved these changes May 16, 2020
@mojoaxel
Copy link
Member

BREAKING CHANGE: This is no longer a drop in replacement for the old 4.21 Vis.js.

I'm confused! To provide a "drop-in replacement" was the whole point of this repository.
Everybody (who can) should use the new libraries. This was especially for the people/projects that can/will not change there code.

Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

This should be discussed further..

@Thomaash
Copy link
Member Author

I get your point and it's a good point. However this broke about 7 months ago (last release) and nobody had the time to fix it. And since we are making more and more breaking changes it will be even more demanding to keep this as a drop in replacement.

@Thomaash
Copy link
Member Author

Awesome work! I think it might be worth changing the commit from "feat" to the tag that changes the major version since the great work done here is a breaking change

Thanks.

Regarding versioning: The prefixes are irrelevant (feat, fix or even perf can be major if appropriate). What causes major version releases is the BREAKING CHANGE: at the start of the commit message body.

mojoaxel
mojoaxel previously approved these changes May 16, 2020
Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

First let me thank you for the work you have done. I just had a more detailed look and it looks good.

My problem is not with breaking changes in functionality (as long as the semver is updated).
I created this repository especially so that users can EASILY update to the new libraries without getting into the whole package-format nightmare.

I thinks we should encourage users to use the separated libraries and not this repository. But I'm sure there are still thousands of users out there that just want to import a JS and a CSS file (Maybe in there python or php project) without thinking about wich file to take.

Maybe you could update the README so it si clear for UMD users wich files to take and there is no longer a seperate CSS-file?

If I had to choose I would try to keep the old rollup-config and not use the super-facy-all-included one for this repository.

Sadly I don't have time at the moment vor visjs so I'm perfectly fine if you guys merge this or any other pull-request! Thanks again!!!

README.md Show resolved Hide resolved
BREAKING CHANGE: This basically reverts the most breaking parts of the
previosly commited overhaul.

Use a compromise solution between backwards compatibility and easy of
maintenance. The old exports are restored and so is the file listing in
the package. However the libraries themselves may still introduce
breaking changes in their features etc.
@Thomaash Thomaash dismissed stale reviews from mojoaxel and yotamberk via 0d8d8e3 May 17, 2020 10:04
Thomaash added 3 commits May 17, 2020 12:07
This way people will hopefully pick the latest version instead of
blindly copypasting 1.0.0.
@Thomaash
Copy link
Member Author

Okay, so I reworked this to a compromise solution. Everything new is simply reexported but the legacy, deprecated and no longer available exports (vis.network, vis.util, vis.DOMutil…) are still available through this. The package format is also unchanged now (except that even CSS has maps but that's not a breaking change).

Regarding the fancy Rollup config: It makes things so much easier and especially changes in config. I change it in one place and the rest is just updates (Renovate handles most of that). Copy pasting the stuff all over the place every time something changes is a nightmare.

@Thomaash Thomaash requested review from mojoaxel and yotamberk May 17, 2020 10:32
Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

LGTM

@Thomaash Thank you for your understanding and your amazing work here!

@yotamberk yotamberk merged commit 5fc2cb2 into master May 17, 2020
@yotamberk yotamberk deleted the overhaul branch May 17, 2020 11:40
@vis-bot
Copy link
Collaborator

vis-bot commented May 17, 2020

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants