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

Move jQuery and Popper to peerDependencies #23244

Merged
merged 4 commits into from
Aug 9, 2017
Merged

Move jQuery and Popper to peerDependencies #23244

merged 4 commits into from
Aug 9, 2017

Conversation

mdo
Copy link
Member

@mdo mdo commented Aug 8, 2017

Fixes #23204.

I've made the change to package.json as @bardiharborow suggested. I've also taken a quick pass at some docs updates to clarify usage around source files, compiled files, and that our JS plugins are optional and depend on jQuery and Popper.js.

@@ -66,6 +66,8 @@
},
"license": "MIT",
"dependencies": {

Choose a reason for hiding this comment

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

Is it necessary to keep an empty dependencies object in package.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not? I did to show we have no explicit dependencies...unsure what the best option is there.

Choose a reason for hiding this comment

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

Oh ok, I'm unsure as well but just wanted to point that out, I don't think it is harmful to keep it there or to remove it.

Copy link

Choose a reason for hiding this comment

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

It's safe to remove dependencies if needed. Some of my libraries do that and it works just fine.

Copy link
Member

Choose a reason for hiding this comment

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

Of course it's safe. It's left on purpose if you read the above comments, to indicate that there are no dependencies.

Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

LGTM

The empty dependencies object is fine, doesn't hurt and in fact npm init adds it too when there are no dependencies IIRC.

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

LGTM 👍
It won't cause any issues with my PR to improve our modularisation 😄

@mdo mdo merged commit 209a963 into v4-dev Aug 9, 2017
@mdo mdo mentioned this pull request Aug 9, 2017
@Johann-S Johann-S deleted the peers branch August 10, 2017 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants