rxaviers / cldrjs

Simple CLDR traverser
MIT License
155 stars 30 forks source link

Problem with babelify: Uncaught TypeError: Cannot read property 'EventEmitter' of undefined #54

Closed wwwouter closed 7 years ago

wwwouter commented 7 years ago

I have the same problem as here: https://github.com/metafizzy/isotope/issues/976

There seem to be two options:

If you don't want to downgrade eventemitter I can try to create a pull request with ev-emitter

Edit: another option could be upgrading eventemitter to the latest version (5.1.0)

rxaviers commented 7 years ago

Hi @wwwouter, thanks for filing this issue.

EventEmitter is (slightly modified and) embedded in cldrjs. I guess the issue could be fixed differently. Could you try the following please?

  1. In your local installed copy, try:

https://github.com/rxaviers/cldrjs/blob/0.4.8/dist/cldr/event.js#L57

- var exports = this;
- var originalGlobalValue = exports.EventEmitter;
+ var exports = {};

https://github.com/rxaviers/cldrjs/blob/0.4.8/dist/cldr/event.js#L486-L494

- /**
-  * Reverts the global {@link EventEmitter} to its previous value and returns a reference to this version.
-  *
-  * @return {Function} Non conflicting EventEmitter class.
-  */
- EventEmitter.noConflict = function noConflict() {
-   exports.EventEmitter = originalGlobalValue;
-   return EventEmitter;
- };
  1. Test it again and if that works we can make that change in the build process of this package: https://github.com/rxaviers/cldrjs/blob/0.4.8/Gruntfile.js#L129-L135

Thanks!

wwwouter commented 7 years ago

Currently I monkey patch it like this by replacing

EventEmitter = (function () { to EventEmitter = (function (exports) {

var exports = this; to //var exports = this;

}()); to }(this || {}));

This is taken from the v5.1.0 of https://github.com/Olical/EventEmitter

Do you think that would be a better option?

rxaviers commented 7 years ago

Thanks for testing it...

Do you think that would be a better option?

In your monkey patch, the exports variable inside the Self-Invoking Anonymous Function got value this. In my suggestion, I'm explicitly setting it to {}.

Since EvenEmitter is embedded, I think isolating it (i.e., using {} instead of this) is better.

Thoughts? Would you be willing to submit a PR to fix the build here? Thanks

wwwouter commented 7 years ago

In this situation {} seems better than this

I created a pull request using {}

rxaviers commented 7 years ago

Awesome! Thanks for finding this issue again and for filing the PR.

ianlamb commented 7 years ago

Bumping this thread as we're running into the same issue trying to use globalize with browserify/babelify. Any chance that PR can be merged in soon and get a release published?

rxaviers commented 7 years ago

Closed by #55

Kynde commented 6 years ago

You do realize that the current globalize still pulls 0.4.8 as a dep and we're not getting the 0.5.0 with this fix?!

That is because the 0.4.8 -> 0.5.0 is major release because it's 0.x.y and the "^0.4.6" does not allow that.

It's just that fixing yarn.lock and package-lock.jsons mangually is tedious.

rxaviers commented 5 years ago

Follow an update: globalize@1.4.2 includes cldrjs@^0.5.0