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

BREAKING CHANGE: drop core-js from dependencies which the app should be responsible for #31

Merged
merged 1 commit into from
Sep 8, 2019

Conversation

gfx
Copy link
Contributor

@gfx gfx commented Jul 3, 2019

Any polyfills, not ponyfills which does not affect the global env, should be injected by the application, not by the library.

If you think this PR is okay, please add "sideEffects": false to package.json.


This is a breaking change (i.e. semver-major) because it may require the application code changes.

@rxmarbles rxmarbles requested a review from okonet July 5, 2019 03:05
@okonet
Copy link
Collaborator

okonet commented Jul 10, 2019

@rxmarbles what do you think? Also I'd like to move this package to react-dropzone org. Thoughts?

@okonet okonet requested a review from rxmarbles July 10, 2019 17:51
Copy link
Collaborator

@rxmarbles rxmarbles left a comment

Choose a reason for hiding this comment

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

@okonet seems reasonable to me. also this repo seems to already be in the react-dropzone org. Do you mean to move this code into react-dropzone directly?

@okonet
Copy link
Collaborator

okonet commented Jul 12, 2019

I moved the repo to the org after I mentioned it. Feel free to move the code along but in this case we might want to create a monorepo structure to accommodate other packages as well?

@gfx
Copy link
Contributor Author

gfx commented Jul 14, 2019

IMO I don't like monorepo because its version-to-version diffs are hard to understand.

@gfx
Copy link
Contributor Author

gfx commented Aug 7, 2019

ping

@okonet
Copy link
Collaborator

okonet commented Aug 7, 2019

I’m not sure if that should be release as a major or minor version. Thoughts?

@gfx
Copy link
Contributor Author

gfx commented Aug 8, 2019

Neither am I, but if I were the product owner I'd bump the major version because some users are required to change their application code.

@rxmarbles
Copy link
Collaborator

I would agree with a major bump since it could be a breaking change for consuming apps

@okonet
Copy link
Collaborator

okonet commented Aug 8, 2019

Okay in this case could you please rename the commit and include the breaking change message according to semantic versioning convention?

@gfx gfx changed the title drop core-js from dependencies which the app should be responsible for BREAKING CHANGE: drop core-js from dependencies which the app should be responsible for Aug 28, 2019
@gfx
Copy link
Contributor Author

gfx commented Aug 28, 2019

Marked it as BREAKING CHANGE both in the PR title and the commit messages. Is it okay?

@okonet okonet merged commit 5d4cdf8 into react-dropzone:master Sep 8, 2019
@okonet
Copy link
Collaborator

okonet commented Sep 8, 2019

I’ve adapted the commit message during the merge to comply semantic-versioning. Hopefully it’s going to be released as a major version. Thanks!

@gfx
Copy link
Contributor Author

gfx commented Sep 9, 2019

Thanks!

@okonet
Copy link
Collaborator

okonet commented Sep 9, 2019

Not sure what's going on but travis CI can't trigger semantic release anymore. Can someone investigate? It says 403 but Im not sure what to do.

@rxmarbles
Copy link
Collaborator

@okonet I'll take a look when I have some free time. Hopefully today.

@gfx gfx deleted the drop_core_js_dependency branch September 10, 2019 16:52
@github-actions
Copy link

🎉 This PR is included in version 2.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.

3 participants