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

Upgrade rollup and plugins to fix the build #6493

Merged
merged 1 commit into from
Sep 4, 2019

Conversation

benmccann
Copy link
Contributor

Closes #6492

etimberg
etimberg previously approved these changes Sep 2, 2019
@benmccann benmccann mentioned this pull request Sep 2, 2019
kurkle
kurkle previously approved these changes Sep 2, 2019
@etimberg
Copy link
Member

etimberg commented Sep 2, 2019

@benmccann https://travis-ci.org/chartjs/Chart.js/builds/579551203#L524 looks like something can't find rollup

@benmccann benmccann dismissed stale reviews from kurkle and etimberg via a272d6c September 3, 2019 01:49
@benmccann benmccann force-pushed the build-fix branch 2 times, most recently from a272d6c to 1a07ab4 Compare September 3, 2019 02:00
@benmccann
Copy link
Contributor Author

I pushed a new version that's working now

Copy link

@gotoken gotoken left a comment

Choose a reason for hiding this comment

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

Looks good to me

@etimberg
Copy link
Member

etimberg commented Sep 4, 2019

Is the package lock needed to fix this? I recall it being controversial in the past

@benmccann
Copy link
Contributor Author

The package-lock makes it so that dependencies are used with consistent versions. Otherwise someone can release a new version of a dependency that's incompatible with our code or another dependency that we use causing the build to spontaneously break. That's exactly what happened here. It also happened in #6039. Adding the package-lock would prevent these types of issues from reocurring

I think the previous concerns revolved around Travis (and many users) using an older version of node/npm that didn't support package-lock. However, that was more than two years ago and we've since updated Travis to use the LTS version of node so I don't expect it to be much of an issue. Worst case if a user is on an older version of node then the package-lock would just be ignored. I closed the earlier PR because of a bug in the initial version of npm, but I've been using it locally since that bug was fixed and haven't encountered any additional issues for the past couple years. There was also a comment on that thread about it being potentially noisy, but the file is only updated when you update package.json, which happens fairly infrequently so I don't see that being an issue in practice

@kurkle
Copy link
Member

kurkle commented Sep 4, 2019

package-lock will not help the same situation in projects using Chart.js as dependency. So the same issue would occur in higher level projects all over, if we did not catch it here.
see

I think @simonbrunel was quite strongly against having it in the repository, but I did not spend the time to find the arguments. (@benmccann did link those in his comment)

@benmccann
Copy link
Contributor Author

That's true only for moment (and chartjs-color, but we control that one completely). The vast majority of our dependencies are dev dependencies which won't get pulled in by projects using Chart.js. The purpose of this PR is to avoid our dev tooling randomly breaking. Both breakages have been due to dev dependencies

@kurkle
Copy link
Member

kurkle commented Sep 4, 2019

Right, so the only concern left is about how frequently we need to update those dev dependencies in the future and will that be noisy. I think that noise is acceptable.

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
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.

Build failed
4 participants