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

Add wrapper to use AMD or browser globals to load plugin (plus tests) #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

neverfox
Copy link

I added a wrapper based on UMD to make the plugin support AMD/RequireJS, and I created a separate AMD test suite. All tests pass. I did not bother, in light of #33, to create tests with ko.mapping. I didn't add CommonJS/Node compatibility, but that would be easy enough to add, if desired.

@coderenaissance
Copy link
Owner

This is under consideration. Off the top of my head I don't see a problem with the change needed to support this. I need to re-familiarize myself with AMD and RequireJS. I've never used them and it's been a while since I looked at them.

One thing I see as problematic is the necessity to duplicate all of the unit tests. I'm currently revising the unit tests which means I'd have to do the changes twice. And every time I add a test I'd have to add it twice. I'd like to be able to have the tests defined once and used to test both. I'll put some thought into it.

Thanks for the help.

@neverfox
Copy link
Author

I'm a heavy RequireJS user so the change was admittedly self-serving, but I also thought it would make it more consistent with the support offered by Knockout's other official plugins (as well as Knockout itself), as well as fitting into frameworks like Durandal. Thank you for considering it.

I thought you might say that about the tests and I did give it some thought at the time. I made the extra set because it was a faster proof-of-concept than writing a script to automatically build the pure AMD test suite from the regular suite. However, I still think the latter is possible (e.g. someone did just that with the entire Twitter Bootstrap library). It's really nothing more than wrapping each module in the same manner and maintaining the "main" file instead of the html file. I'll put some thought too into automating the build of the AMD suite.

@coderenaissance
Copy link
Owner

Thanks.

@thelinuxlich
Copy link

+1

@aknuds1
Copy link

aknuds1 commented Aug 2, 2013

I would like to see AMD support as well, as this plugin doesn't work out of the box with Knockout and RequireJS (since Knockout then doesn't create a global ko object).

@coderenaissance
Copy link
Owner

working on the next release which has some notable changes... will try to get it in this release if it's simple, if not will be small release just after... have been delaying too long on this release already. Thanks for your help.

@spring1975
Copy link

Yes! AMD support please (specifically RequireJS). Haven't been able to get shim config to play nice with this plug-in, so had to modify the library to make it work.

@jnicolau
Copy link

+1

1 similar comment
@bjarneheden
Copy link

+1

@legacycode
Copy link

+1 for AMD support

@jotunskij
Copy link

+1

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.

9 participants