search

stefanpenner / es6-promise

A polyfill for ES6-style Promises
MIT License
7.3k stars 593 forks source link

Auto Bundles were not working in AMD Environment #263

Closed azizhk closed 7 years ago

azizhk commented 7 years ago

This was because of this code: https://github.com/stefanpenner/es6-promise/blob/f5f44e81e3d92db1445d61c157a3593a3662ef81/dist/es6-promise.auto.js#L11-L12

ES6Promise would not be defined on global and we would get an error saying, "Cannot read property polyfill of undefined". Though there are multiple workarounds, I thought this would be the best way to fix this.

I would have preferred to export nothing, just call the polyfill function, which would make it inline in the minified version, but that would break semver. Also I would love to change the module name from ES6Promise to es6-promise, to keep it the same in NPM and on the browser. (But that would make it window['es6-promise'] for non AMD 😢 ) Also this is my first time with Broccoli.js. So please correct me if I'm doing something wrong. Also I did not commit all of the dist files. Let me know if I should not commit them.

stefanpenner commented 7 years ago

@azizhk thanks for digging into this. Do you think we should just always install the ES6Promise global even in AMD environments? I suspect the lack of this may be causing https://github.com/stefanpenner/es6-promise/issues/257 as well, as causing this issue.

What do you think?

azizhk commented 7 years ago

Do you think we should just always install the ES6Promise global even in AMD environments?

If you mean we should just set window.ES6Promise then no, we should not. If you mean we should polyfill it then yes. He is requesting for the auto version.

stefanpenner commented 7 years ago

If you mean we should just set window.ES6Promise then no, we should not.

👍

If you mean we should polyfill it then yes. He is requesting for the auto version.

Good catch.

stefanpenner commented 7 years ago

I'm trying to replicate the above test failure locally, but without much luck...

azizhk commented 7 years ago

Same here. I was pretty sure they were working when I opened my PR. Damn.

stefanpenner commented 7 years ago

I'm rebuilding master on travis, lets see if its node 4 also fails. If so, then maybe its some transitive?

I really wish I could replicate locally...

stefanpenner commented 7 years ago

Ya, it failed again. I really wish it was easily replicatable.

I am at work right now, I'll have to take deeper look later...

stefanpenner commented 7 years ago

Im really not sure whats up. I have tested this thoroughly locally in node 4 (the failing test) without any issues.. Going to merge this.

stefanpenner commented 7 years ago

moving node 4 to allowed failures for now: https://github.com/stefanpenner/es6-promise/pull/272

Will test node 4 locally to confirm until we resolved the node 4 issue.