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 root to factory function when loading with modules #180

Merged
merged 7 commits into from
Sep 18, 2018

Conversation

jfspencer
Copy link
Contributor

@ulfryk @cwmyers this change gets Monet.js working in my ES6 project, tests are passing. Is root expected to be window in browser contexts? let me know if I need to go in a different direction and I will update the PR.

@ulfryk
Copy link
Member

ulfryk commented Sep 12, 2018

@jfspencer - thanks for contribution :)

Your direction seems to be 100% OK.

Just please fix issues that break tests. And make us 100% sure that test on window will not 💥 on window detection. I have a feeling that there is some possibility that try / catch clause may be needed instead of ternary operator.

@ulfryk ulfryk self-requested a review September 12, 2018 04:56
@ulfryk ulfryk added this to the 0.9 Release milestone Sep 12, 2018
src/monet.js Outdated
@@ -14,7 +14,7 @@
if (typeof define === 'function' && define.amd) {
define(factory)
} else if (typeof module === 'object' && module.exports) {
module.exports = factory()
module.exports = factory(!!window ? window : root)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window || root will be shorter.

@jfspencer
Copy link
Contributor Author

@ulfryk updated to use try/catch, with passing the existing tests. not sure how to unit test this though 🤔

@ulfryk
Copy link
Member

ulfryk commented Sep 13, 2018

@jfspencer - to be sure if we're doing it right I searched for info about UMD patterns and found that it all can be actually done much simpler :)
See: https://github.com/umdjs/umd
Especially: https://github.com/umdjs/umd/blob/master/templates/returnExports.js

so:

    
    } else if (typeof module === 'object' && module.exports) {
        module.exports = factory(root) //yeah, just root instead of window
    } else {
    
  // and this tricky ternary here instead of just this
}(typeof self !== 'undefined' ? self : this, function (rootGlobalObject) {

and please update also src/monet-pimp.js

Sorry for confusion - this part with UMD is in my opinion not a thing that coders should do (and I'm never up to date with it) - language should care about it :)

We should move away from writing it by ourselves here and probably switch to Rollup or webpack ( #181 )

@jfspencer
Copy link
Contributor Author

@ulfryk Good to know, thanks for clarifying that. updated the PR accordingly.

@ulfryk ulfryk merged commit bd53a58 into monet:develop Sep 18, 2018
@ulfryk ulfryk mentioned this pull request Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants