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

set jQuery as a peer dependency #2248

Merged
merged 1 commit into from Jan 26, 2019
Merged

Conversation

aigdonia
Copy link
Contributor

@aigdonia aigdonia commented Dec 16, 2018

After bundle withwebpack this package includes its own jquery to the bundle and no way to get rid of it except by add jquery as a peerDependency to the package.json

I found a non merged PR from 2016 here #1858 but setting as a devDependency is not the best fit, so I created this PR

thanks

@staabm
Copy link
Member

staabm commented Dec 16, 2018

Thx for the PR.

I had to lookup what peerDependencies are.
I came up with https://nodejs.org/en/blog/npm/peer-dependencies/

It reads like this is usefull for our library use-case.

@Arkni what do you think?

@Arkni
Copy link
Member

Arkni commented Dec 16, 2018

I'm +1 to this change. But, is it ok to release this in a minor/patch version instead of a major one?

@aigdonia
Copy link
Contributor Author

any update to decision here?

@Arkni
Copy link
Member

Arkni commented Jan 20, 2019

Hi @aigdonia,

Sorry for taking so long to respond, the last month (and still have limited time) was crazy for me. As I said before, I'm +1 to the idea of removing jQuery from the dependencies and moving it to peer dependencies. The only thing I lack information on is should we release this as part of a patch/minor version or a major one?

If you have any information/recommendation about that, please feel free to provide them.

Thanks a lot for working on this and thanks for your patience :)

@aigdonia
Copy link
Contributor Author

as per the definition of semver.org
a MAJOR release should make incompatible API changes, which is not the case here,
this change won't affect the current installs of the package as well,
the only effect I see it when new npm install is introduced without jquery being introduced already in the project, there will be a warning to install it.

so I suggest this to be a minor/patch release.

@Arkni
Copy link
Member

Arkni commented Jan 26, 2019

A minor release it is then.

Thanks @aigdonia for the information :) I'm going to merge this proposal in a moment.

Thanks :)

@Arkni Arkni merged commit 01ca879 into jquery-validation:master Jan 26, 2019
@staabm
Copy link
Member

staabm commented Jan 26, 2019

Thank you guys 😎

@aigdonia
Copy link
Contributor Author

Thanks for the merge

@ericfowler303
Copy link

Would it be possible to publish a beta build or public build of this to bring in via npm? I'm currently having issues with this using webpack due since this commit isn't in a built package.

@tommyip
Copy link

tommyip commented Jun 4, 2019

Until a new build is published, simply add the following to package.json to fix the problem:

"resolutions": {
    "jquery-validation/jquery": "3.4"
}

(Thanks @marcwieland95 for coming up with the fix in #2272)

@pam81
Copy link

pam81 commented Jun 15, 2019

I tried this solution but it's still using the jquery from the inside node_module folder.
The only way i found a solution is to removed this folder after doing yarn install.

@staabm
Copy link
Member

staabm commented Jun 15, 2019

I just published jquery-validateion 1.19.1 to npm.

most important change is that jquery is now defined as a peer dependency, so we dont get into your way. see #2248

sorry this took us so long. jq-validation is not top priority atm

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.

None yet

6 participants