Skip to content

Build: add UMD support #206

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

Closed
wants to merge 2 commits into from
Closed

Build: add UMD support #206

wants to merge 2 commits into from

Conversation

niksy
Copy link
Contributor

@niksy niksy commented Jul 7, 2016

This PR implements UMD support as discussed in #98 regarding "Support CJS internally".

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.288% when pulling ae9a49c on niksy:umd-support into 93c37d1 on jquery:master.

@dmethvin
Copy link
Member

dmethvin commented Jul 7, 2016

Awesome, thanks!

@dmethvin
Copy link
Member

dmethvin commented Jul 7, 2016

We may want to add return jQuery in outro.js so that it will return the modified jQuery when used with CJS. It's going to actually modify its incoming jQuery arg, but it seems like using the return value might be more natural. What do you think?

@niksy
Copy link
Contributor Author

niksy commented Jul 9, 2016

@dmethvin yeah, that makes sense! Do you think these change should be applied to 1x-stable branch, and not only to jQuery v3?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.288% when pulling a744cec on niksy:umd-support into 93c37d1 on jquery:master.

@dmethvin
Copy link
Member

Yes, I can cherry pick it into that branch as well and do new releases for both.

@niksy
Copy link
Contributor Author

niksy commented Jul 10, 2016

OK, should I squash those 2 commits before merge?

@dmethvin
Copy link
Member

I can squash them when I land it.

@dmethvin dmethvin mentioned this pull request Jul 11, 2016
@dmethvin dmethvin closed this in f92c743 Jul 12, 2016
dmethvin pushed a commit that referenced this pull request Jul 12, 2016
Fixes #207
Closes #206

(cherry picked from commit f92c743)
@mgol
Copy link
Member

mgol commented Aug 31, 2016

Why is jquery a dependency and not a peerDependency? This seems wrong as Migrate should be always patching an existing jQuery copy and not provide its own.

@dmethvin
Copy link
Member

dmethvin commented Sep 8, 2016

I remember thinking about this at the time, the issue with peerDependencies is that npm3 requires you to install them manually and I didn't wan't to do that. Maybe there is another way out? Perhaps just devDependency, since the Migrate code itself dynamically checks that there is a jQuery core loaded.

@mgol
Copy link
Member

mgol commented Sep 12, 2016

I remember thinking about this at the time, the issue with peerDependencies is that npm3 requires you to install them manually and I didn't wan't to do that.

There's no way around that. The purpose of peerDependencies is to depend on a package that needs to have one instance and you can't create your own. The correct way to go is to put it both in devDependencies and peerDependencies (the version range in peerDependencies should be more forgiving) and remove from dependencies.

dmethvin added a commit to dmethvin/jquery-migrate that referenced this pull request Sep 27, 2016
dmethvin added a commit that referenced this pull request Sep 30, 2016
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