Skip to content

Adding module export support outside of stomp.coffee #92

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 10 commits into from
Closed

Adding module export support outside of stomp.coffee #92

wants to merge 10 commits into from

Conversation

williamsbdev
Copy link
Contributor

I noticed on #88 that you had desired for the module exports to be outside of the stomp.coffee. This is a very breaking change as I did that and would require a major version bump. However, I added the ability to also export as an AMD module. There is some other cleanup as well. Please let me know if you would like things changed or how you would like to proceed.

Thanks for this awesome project!

@williamsbdev
Copy link
Contributor Author

@jmesnil I was just following up on this PR to see if you have had a chance to look at this yet and if so, is it acceptable? I am willing to work with you to help change this as needed to make it meet your requirements.

Thank you again for this project!

@williamsbdev
Copy link
Contributor Author

@jmesnil Is this PR something you would like to pull in? If so, is there something I need to do to help this get pulled in?

@jmesnil
Copy link
Owner

jmesnil commented Dec 2, 2014

sorry for the late reply, I don't have a lot of spare cycles at the moment (having a full time job and being a father of a 2 month old baby) :)

I prefer to be very conservative on that project as it is quite stable. There should be no major version changes due to a refactoring (I don't expect to have another major version unless there is a new version of STOMP).
I regret having added commonjs support directly inside stomp.js but I prefer to keep it there (as well as Web Browser support through the window object) instead of having a major change just to remove it.
If you could update your PR to add AMD support without touching stomp.coffee, that'd be great.

Otherwise, your changes to the build process looks good.
Could you also add some doc on how to use the AMD module (I'm not familiar with it)?

Thanks!

@williamsbdev
Copy link
Contributor Author

I'm going to close this issue. Adding AMD support without modifying the stomp.coffee will not give me the desired outcome. Because of the code that is putting stomp on window, even if I were to wrap the output into an AMD module, it would still be available globally and would therefore defeat the purpose of any PR I could submit.

Thanks for the project. We will just fork it and modify it so that we have an AMD module only.

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.

2 participants