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 to peerDependencies #1414

Merged
merged 2 commits into from
Jul 11, 2018
Merged

Conversation

graingert
Copy link
Contributor

@graingert
Copy link
Contributor Author

Without this, it can cause selectize to try to use a different jquery than the main jquery

@graingert
Copy link
Contributor Author

@joallard currently selictize is locked to jQuery >=1.7.0 <2.0.0 for npm users. This pr opens that range up to include v2 and v3, and also ensures that only one version of jQuery is installed and used.

@graingert
Copy link
Contributor Author

Fixes: #1277 I think

@matason
Copy link

matason commented Jul 6, 2018

Thanks @graingert, this fixes it for me.

@Kovah
Copy link

Kovah commented Jul 8, 2018

This would also solve this jQuery security vulnerability, which is patched in versions >= 3.0.0.

@joallard joallard merged commit 5bf6607 into selectize:master Jul 11, 2018
@graingert
Copy link
Contributor Author

graingert commented Jul 11, 2018 via email

@joallard
Copy link
Member

Done. 0.12.6

@Pictor13
Copy link
Contributor

Pictor13 commented Jul 13, 2018

Great!

About jQuery, do we need to keep it in bower.json with that version range, for any retro-compatibility reason, or could we even remove it from there and rely on bower-away to move all the dependencies to package.json (Yarn/Npm) ?
Or do it manually, without bower-away.

Correct me if I am wrong, but I see no reason for having two conflicting definitions in bower and npm; is there?

@graingert
Copy link
Contributor Author

graingert commented Jul 13, 2018 via email

@Pictor13
Copy link
Contributor

Looks like there is an issue with forcing the jQuery version to ^3.3.1 (#1428).

I guess peerDependencies wins over dependencies and doesn't retain the ^1.7 compatibility.

@Pictor13
Copy link
Contributor

Pictor13 commented Jul 16, 2018

graingert wrote:

Use boweraway there's no need for bower any more

I just want to be sure that we are not leaving anybody down in removing bower (old setups? other cases?).
And (I guess) we should anyway keep the bower.json manifest, so that it's still up to the developer to choose to search the package over the bower registry or download it from there? Am I wrong?

Am not sure that installation will still work without any specified bower dependency; but maybe it's fine anyway (should be tested).

Don't we want that users that were using bower install (or have scripts using that) could keep doing so?
The difference would be just that it delegates to npm the role of single-source-of-truth for the dependencies.
(I guess there should be a way to tell bower to rely on npm/yarn, when installing; am still looking at how it could work)

EDIT:
maybe I should move this discussion to a separate issue? 😅

@graingert graingert deleted the patch-1 branch July 16, 2018 16:01
Pictor13 added a commit to Pictor13/selectize.js that referenced this pull request Jul 17, 2018
Pictor13 added a commit to Pictor13/selectize.js that referenced this pull request Jul 17, 2018
- Logical OR should be used in semver, rather than comma for logical AND.

    (https://getcomposer.org/doc/articles/versions.md#version-range)
    (https://semver.npmjs.com/)

- Resolves the UNMET PEER DEPENDENCY [email protected]

    No more "npm ERR! peer dep missing: jquery@^1.7.0, ^2, ^3, required by [email protected]"

    'npm ls' on projects now does not report UNMET PEER DEPENDENCY anymore.

fixes selectize#1414
@joallard
Copy link
Member

Phasing out bower is something I agree we should go in the direction of, I'd be receptive to such PRs

michael-maltsev pushed a commit to michael-maltsev/selectize.js that referenced this pull request Aug 28, 2018
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.

5 participants