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

Added UMD support fixes #977 #1106

Merged
merged 1 commit into from May 21, 2014

Conversation

Projects
None yet
3 participants
@larslaade
Contributor

larslaade commented May 4, 2014

I've added UMD Support with the UMD Plugin Wrapper.
It's done via the build process, so you don't have the wrapper in all files.

Many files needed changes:

  • use $ instead of jQuery
  • removed the self-executing function wrappers

Localization files are now copied to dist/ to add the wrapper.

The release task is updated to use the new created files.

The additional methods are currently merged into one file with the wrapper. Maybe this could be changed in the future to have all files separated in one folder like the localization files.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer May 6, 2014

Collaborator

Thank you, this is looking pretty good already. I noticed two things so far:

  • The uglify task now overwrites the dist files, instead of creating separate .min.js files. All for should be created though, see also build/release.js
  • The requirejs demo should be added to demo/index.html. You could also simplify the requirejs config there, by simply specifying "jquery": "../../lib/jquery-1.9.1", removing themap` property. Also, with jQuery including a define call, is the shim property really necessary?
Collaborator

jzaefferer commented May 6, 2014

Thank you, this is looking pretty good already. I noticed two things so far:

  • The uglify task now overwrites the dist files, instead of creating separate .min.js files. All for should be created though, see also build/release.js
  • The requirejs demo should be added to demo/index.html. You could also simplify the requirejs config there, by simply specifying "jquery": "../../lib/jquery-1.9.1", removing themap` property. Also, with jQuery including a define call, is the shim property really necessary?
@larslaade

This comment has been minimized.

Show comment
Hide comment
@larslaade

larslaade May 6, 2014

Contributor

uglify task: separate .min.js files would need another umd wrapper/config in those files to load the correct jquery-validate.min.js.

// non minified
define( ["jquery", "../jquery.validate"], factory );
// minified
define( ["jquery", "../jquery.validate.min"], factory );

Maybe the release/uglify taks should create a new folder for all minified files?

I will look at the demo soon and change that.

Contributor

larslaade commented May 6, 2014

uglify task: separate .min.js files would need another umd wrapper/config in those files to load the correct jquery-validate.min.js.

// non minified
define( ["jquery", "../jquery.validate"], factory );
// minified
define( ["jquery", "../jquery.validate.min"], factory );

Maybe the release/uglify taks should create a new folder for all minified files?

I will look at the demo soon and change that.

@larslaade

This comment has been minimized.

Show comment
Hide comment
@larslaade

larslaade May 7, 2014

Contributor

The uglify task creates separate .min.js files now. I have added a new grunt plugin to replace the define dependencies with the path to the .min.js files.

The RequireJS demo is updated too.

Contributor

larslaade commented May 7, 2014

The uglify task creates separate .min.js files now. I have added a new grunt plugin to replace the define dependencies with the path to the .min.js files.

The RequireJS demo is updated too.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer May 21, 2014

Collaborator

@larslaade thanks for the updates. This is looking pretty good. Could you help out a bit further by rebasing this on top of master? There will be several merge conflicts due to other changes that landed in the mean time. A squash of all the commits would be good as well. The commit message should follow the guidelines, but that's the least issue you should worry about. Thanks again.

Collaborator

jzaefferer commented May 21, 2014

@larslaade thanks for the updates. This is looking pretty good. Could you help out a bit further by rebasing this on top of master? There will be several merge conflicts due to other changes that landed in the mean time. A squash of all the commits would be good as well. The commit message should follow the guidelines, but that's the least issue you should worry about. Thanks again.

@larslaade

This comment has been minimized.

Show comment
Hide comment
@larslaade

larslaade May 21, 2014

Contributor

@jzaefferer now it is squashed and rebased

Contributor

larslaade commented May 21, 2014

@jzaefferer now it is squashed and rebased

@jzaefferer jzaefferer merged commit db55d36 into jquery-validation:master May 21, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer May 21, 2014

Collaborator

Awesome, thank you for the quick update. This is really nice, so much better then having the wrappers everywhere in the source code.

Collaborator

jzaefferer commented May 21, 2014

Awesome, thank you for the quick update. This is really nice, so much better then having the wrappers everywhere in the source code.

@nschonni nschonni referenced this pull request Jun 23, 2014

Closed

AMD compatibilty #1178

@szepeviktor

This comment has been minimized.

Show comment
Hide comment

szepeviktor commented Jun 23, 2014

@szepeviktor

This comment has been minimized.

Show comment
Hide comment

szepeviktor commented Jun 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment