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

Initial attempt at AMD support #99

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@lboynton
Contributor

lboynton commented Nov 28, 2012

This is my initial attempt at AMD support for issue #66, with supporting tests. I've made some compromises to make the diff smaller. Ideally, the source files should be renamed to match the module name (ie Base64.js instead of base64.js) so that you don't need the path mapping in requirejs.config. I've also added an example using AMD.

I will squash the commits before merging once getting some feedback.

@aredridel

This comment has been minimized.

Show comment
Hide comment
@aredridel

aredridel Nov 28, 2012

Isn't this usually just define(factory)?

aredridel commented on src/base64.js in 6571e3c Nov 28, 2012

Isn't this usually just define(factory)?

This comment has been minimized.

Show comment
Hide comment
@lboynton

lboynton Nov 28, 2012

Owner

Yes, I later noticed that you didn't have to pass an array as the first parameter if there are no dependencies. Will update.

Owner

lboynton replied Nov 28, 2012

Yes, I later noticed that you didn't have to pass an array as the first parameter if there are no dependencies. Will update.

@aredridel

This comment has been minimized.

Show comment
Hide comment
@aredridel

aredridel Nov 28, 2012

It might make more sense to use almond.js for testing -- it's a tiny little shim that doesn't do any actual resolution.

aredridel commented on tests/require.js in 6571e3c Nov 28, 2012

It might make more sense to use almond.js for testing -- it's a tiny little shim that doesn't do any actual resolution.

@aredridel

This comment has been minimized.

Show comment
Hide comment
@aredridel

aredridel commented on cafb375 Nov 28, 2012

Awesome.

@aredridel

This comment has been minimized.

Show comment
Hide comment
@aredridel

aredridel Nov 28, 2012

should this be a var here, or should it be exported later, dependent on amd?

aredridel commented on src/sha1.js in 7879771 Nov 28, 2012

should this be a var here, or should it be exported later, dependent on amd?

This comment has been minimized.

Show comment
Hide comment
@aredridel

aredridel Nov 28, 2012

Oh, I see. A later commit does just that. Nice.

aredridel replied Nov 28, 2012

Oh, I see. A later commit does just that. Nice.

@lboynton

This comment has been minimized.

Show comment
Hide comment
@lboynton

lboynton Dec 3, 2012

Contributor

Should Strophe still define window globals even if AMD is used? There are a lot of Strophe plugins which expect globals. I've found that using the shim config in require.js is unreliable for Strophe plugins which expect the Strophe global. The amdWebGlobal.js below defines a global even if AMD is being used.

https://github.com/umdjs/umd/blob/master/amdWeb.js
https://github.com/umdjs/umd/blob/master/amdWebGlobal.js

Contributor

lboynton commented Dec 3, 2012

Should Strophe still define window globals even if AMD is used? There are a lot of Strophe plugins which expect globals. I've found that using the shim config in require.js is unreliable for Strophe plugins which expect the Strophe global. The amdWebGlobal.js below defines a global even if AMD is being used.

https://github.com/umdjs/umd/blob/master/amdWeb.js
https://github.com/umdjs/umd/blob/master/amdWebGlobal.js

@jfmoy

This comment has been minimized.

Show comment
Hide comment
@jfmoy

jfmoy Jan 9, 2013

A quick message to show my support to the initiative, I hope soon Strophe will be AMD ready.

jfmoy commented Jan 9, 2013

A quick message to show my support to the initiative, I hope soon Strophe will be AMD ready.

@lboynton lboynton referenced this pull request Sep 15, 2013

Closed

AMD Support #29

@lboynton lboynton closed this Dec 8, 2014

Gordin pushed a commit to Gordin/strophejs that referenced this pull request Jan 31, 2015

Merge pull request #99 from Sudrien/master
escapeNode & unescapeNode null tolerance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment