search

paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 387 forks source link

ES6 Shim fails in Bundle #392

Open SebastianStehle opened 8 years ago

SebastianStehle commented 8 years ago

Hi, I created a bundle with es6-shim as first file.

Unfortunatly my app (http://mobile.green-parrot.net/) fails, because of this line of code.

3215: if (!ES.IsCallable(globals.Reflect[key])) { }

The reason is that globals.Reflect is undefined from beginning.

ljharb commented 8 years ago

@SebastianStehle i see you closed this; were you able to resolve the issue? https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L263-L266 defines globals.Reflect in the beginning.

SebastianStehle commented 8 years ago

Partially, I referenced the es6-shim from a cdn and it works. I saw the lines of code you mentioned and I debugged through it, but it was very strange. Because globals.Reflect exists and was undefined your code didnt create a valid reference.

ljharb commented 8 years ago

Are you saying that the last published version works, but master fails?

SebastianStehle commented 8 years ago

No, it doesnt work for me when I bundle it. When I reference es6-shim.js as standalone it works.

ljharb commented 8 years ago

es6-shim is required to be run such that this is the global object, and should be the first code executed on your page (after the es5-shim). It should only be the case that it doesn't work if your bundling pipeline isn't packaging it correctly.

stroborobo commented 8 years ago

I can reproduce this, cating multiple files into one where es6-shim is the first one breaks. Shouldn't that be the same as including those files in <head> in the same order? No fancy bundling.

ljharb commented 8 years ago

@stroborobo if you are naively concatenating multiple files together without even properly separating them with semicolons, a lot of things will break, due to ASI rules. Simple cating has never been a correct or safe approach, and at this point is a very bad web dev practice. In 2016, bundling and/or a build process is simply required.

stroborobo commented 8 years ago

Hey Jordan, I'm very much aware of that :) I wasn't talking about syntax errors or similar. But you're right of course, I just wanted something quick up and running, so I could dabble with the things I was interested in right now. (Eventually use SystemJS, but well, one step at a time, still new to this modern Javascript dependency management.)

But to have that written down: it's Angular2's polyfills that declared Reflect in global space.

Thanks anyway and have a great day! :)

ljharb commented 8 years ago

@stroborobo Angular2 is big enough that I'd be willing to make a small patch to handle that case. However, https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L264-L267 should be working with it. If you can make me a jsfiddle that reproduces the problem I'll be happy to look into it further.

stroborobo commented 8 years ago

Ok, the exact problem is this: angular2-polyfills.js has a var Reflect; in global space, which results an undefined window.Reflect. Your check !globals.Reflect is true but in defineProperty the check is actually name in object which is true, since it's declared but not defined to something evaluating to true.

If you really need a jsfiddle I'd make one, but I guess that's a fairly simple problem.

EDIT: whatever. http://tmp.kbct.de/es6-shim/

ljharb commented 8 years ago

Aha, thank you. Seems simple enough, I'll put up a fix.

ljharb commented 8 years ago

also, it would be good to file a bug report on angular for that behavior - they shouldn't be a) creating globals nor b) using names that are part of the language.

stroborobo commented 8 years ago

I'll have a look into their source code, maybe it's just the Typescript compiler outputting things that weren't intended. Thanks for adding this workaround!

SebastianStehle commented 8 years ago

Hi, thank you for your fix, I finally tried it again, but I have another exception now:

Uncaught TypeError: Cannot redefine property: Reflect

ljharb commented 8 years ago

@SebastianStehle any chance you could make me a jsfiddle that reproduces the issue?

HafizAhmedMoon commented 8 years ago

@SebastianStehle keep the angular2-polyfills at above from es6-shim

  <script src="node_modules/angular2/bundles/angular2-polyfills.js"></script>
  <script src="node_modules/es6-shim/es6-shim.js"></script>
  <script src="node_modules/systemjs/dist/system-polyfills.js"></script>

  <script src="node_modules/systemjs/dist/system.src.js"></script>
  <script src="node_modules/rxjs/bundles/Rx.js"></script>
  <script src="node_modules/angular2/bundles/angular2.dev.js"></script>
ljharb commented 8 years ago

That's not a good solution, es5-shim and es6-shim should always be first :-)

HafizAhmedMoon commented 8 years ago

But, It's working...

ljharb commented 8 years ago

lots of things "work" :-) if there's a bug in angular 2's polyfills, i'd prefer es6-shim to be robust against it if possible, and better, i'd prefer angular 2 fix their broken code.

HafizAhmedMoon commented 8 years ago

You have to change this from

var defineProperty = function (object, name, value, force) {
    if (!force && name in object) { return; }
    if (supportsDescriptors) {
      Object.defineProperty(object, name, {
        configurable: true,
        enumerable: false,
        writable: true,
        value: value
      });
    } else {
      object[name] = value;
    }
  };

to

var defineProperty = function (object, name, value, force) {
    if (!force && name in object) { return; }
    if (supportsDescriptors && Object.isExtensible(object[name])) { // because Reflect in window but undefined and non-extensible
      Object.defineProperty(object, name, {
        configurable: true,
        enumerable: false,
        writable: true,
        value: value
      });
    } else {
      object[name] = value;
    }
  };

it's working.

ljharb commented 8 years ago

@hafizahmedattari is that because angular 2's polyfill makes it non-extensible (which is a violation of the spec)?

HafizAhmedMoon commented 8 years ago

@ljharb you are right, but if you want to

es6-shim to be robust against it

you have to do like this :)

ljharb commented 8 years ago

Not necessarily - even if it's made non-extensible I can still just delete it, and copy its properties over to a new object.

ljharb commented 8 years ago

When I use https://cdnjs.cloudflare.com/ajax/libs/angular.js/2.0.0-beta.7/angular2-polyfills.js - I can't reproduce this whether it's included before or after es6-shim. Is there a specific browser this fails in?

HafizAhmedMoon commented 8 years ago

This error occurs after merging files into one.

ljharb commented 8 years ago

@hafizahmedattari that doesn't make any sense - short of ASI issues (the shims use a UMD so that doesn't apply) and strict mode, two script tags are identical to two concatenated files from a runtime standpoint.

HafizAhmedMoon commented 8 years ago

@ljharb I had debugged the result of globals.Reflect at Line:265 in both situations(separate files and bundle file). The result was shock, with separate files the value of globals.Reflect is object and with bundle file the value of globals.Reflect is undefined. That's why it goes to redefining Reflect, which is already defined in globals with undefined value.

ljharb commented 8 years ago

If you could provide a jsfiddle (or similar) with the bundle file that would be very helpful

SebastianStehle commented 8 years ago

I just reopened the issue, because it is not solved yet. I hope it is okay.

ljharb commented 8 years ago

Absolutely :-)

However, I will say that while I'm very curious to figure it out, the implication is that there's a flaw in your bundling process, since the files work separately - and any time bundling changes behavior it's usually a bundling bug.

cezarykluczynski commented 8 years ago

I was doing Angular2 hello world and stumbled upon this problem too, when trying to concatenate all required dependencies into one files. My solution was to wrap content of every file into self-executing anonymous function.

HafizAhmedMoon commented 8 years ago

Issue resolved in updated angular2 lib, I tried in angular2@2.0.0-beta.12, working without any problem.

jdelobel commented 8 years ago

Same problem with angular2@2.0.0-beta.12 for me (during minification process).

ljharb commented 8 years ago

@jdelobel can you make me a jsfiddle that reproduces it with the latest version of the shim?

jdelobel commented 8 years ago

@ljharb You can clone https://github.com/jdelobel/angular2-tour-of-heroes/tree/develop. Then:

Browser: Chromium Version 47.0.2526.106 Built on Ubuntu 14.04, running on LinuxMint 17.3 (64-bit)

DennisDel commented 8 years ago

Same problem with latest version. Uncaught ReferenceError: a is not defined. Please see the attachment for problem line. _title_

ljharb commented 8 years ago

@DennisDel This looks like a bad minification - it's changing "Map" to "a", but "a" should only be available inside the constructor. Is that the minified build, or if not, what tool are you using to produce that?

DennisDel commented 8 years ago

@ljharb I am using VS 2015 and used Google Chrome pretty print just to show you what the problem is a bit more specifically. I am not using Angularjs 2.0 btw.

ljharb commented 8 years ago

@DennisDel VS 2015 is an editor - what i meant was, are you using the minified build included in this repo, or is your app doing the minification itself?

DennisDel commented 8 years ago

@ljharb I tried minified and normal from this repo. they both have the same problem when I build them in release mode.

ljharb commented 8 years ago

@DennisDel thanks - i think you have a separate issue. would you file a new one with this information, as well as what build process you're using? (if that's something built into VS 2015, mention that too, it may have a bug)

DennisDel commented 8 years ago

Will do. Thank you.

NoMan2000 commented 8 years ago

I also encountered this. When using the ES6 Sham, (but not the ES5 Shim/Sham and ES6 Shim), whenever I minified the files using Uglifier, Safari started throwing errors.

This is the part it finds objectionable, TypeError: Attempting to change configurable attribute of unconfigurable property.

             }, set instanceof Object ? setPrototypeOf = createAndCopy : (set.__proto__ = objProto, setPrototypeOf = set instanceof Object ? function(origin, proto) {
                return origin.__proto__ = proto, origin
            } : function(origin, proto) {
                return getPrototypeOf(origin) ? (origin.__proto__ = proto, origin) : createAndCopy(origin, proto)
            })
        }
        Object.setPrototypeOf = setPrototypeOf
    }
}(), supportsDescriptors && "foo" !== function() {}.name && Object.defineProperty(Function.prototype, "name", {

It's the Object.defineProperty on "foo" !== function that it seems to take umbrage with.

ljharb commented 8 years ago

@NoMan2000 if you're using uglifier, by default it mangles function names, which is why i provide a pre-uglified es6-shim.min.js. To unbreak you, you can use the uglifier option to not do that. (Namely, the line should be }(), supportsDescriptors && "foo" !== function foo() {}.name && Object.defineProperty(Function.prototype, "name", {)

DennisDel commented 8 years ago

Hey all, if you have similar problems, make sure you always use the minifed version and DO NOT use JsMinify to do it. I tried both normal and minified version and they always cause issues with JsMinify.

DennisDel commented 8 years ago

@ljharb Do I need any other file to include in my bundle or es6-shim.min.js alone (and of course the def file) will suffice ?

DennisDel commented 8 years ago

This is my current problem. It is on IE11 _title_

ljharb commented 8 years ago

@DennisDel this RangeError is wrapped by a function that does try/catch - I'm sure you'll find it called a in that screenshot, somewhere in the code - so I'm not sure how that error isn't being caught. Can you make a jsfiddle that reproduces it?

(es6-shim is recommended to follow es5-shim, since every browser violates ES5 in some way, but it should work just fine without it too)

ps37 commented 7 years ago

@ljharb @HafizAhmedMoon I am still getting the following error only on IE11. SCRIPT5078: Cannot redefine non-configurable property 'Reflect'

I am bundling the es6-shim.min.js before all my sources and I am using the latest version of Angular. Is the issue resolved?

ljharb commented 7 years ago

@ps37 Can you try including es6-shim in a separate bundle, before Angular, and make sure all script tags are either omitting or including the defer attributes, and all omitting async? (ie, so all the tags execute in order)