aurelia / polyfills

The minimal set of polyfills needed to run Aurelia.
MIT License
26 stars 27 forks source link

Assigning null arguments causes errors #8

Closed Souldrinker closed 8 years ago

Souldrinker commented 8 years ago

Not sure how to really describe my issue, but when I remove core-js from my application it fails on IE11 with Object.keys: argument is not an Object. It still works fine in Chrome, but to make it work in IE I have to re-enable core-js in my index.html.

In IE without core-js I get the error on the last line in the loop on my last argument (of five) which is null:

          for (var arg, i = 1; i < arguments.length; i++) {
            arg = arguments[i];
            O.keys(arg).concat(filterOS(arg)).forEach(set);
          }

In the call stack I can see that the call came from aurelia-fetch-client's buildRequest function, but don't really see where that is called from:

    var bodyObj = body ? { body: body } : null;
    var parsedDefaultHeaders = parseHeaderValues(defaults.headers);
    var requestInit = Object.assign({}, defaults, { headers: {} }, source, bodyObj);

I had a quick chat with @stoffeastrom that suggested I should post an issue here and that the arg probably should default to empty, Object.keys(arg || {}) to avoid these kind of issues.

EisenbergEffect commented 8 years ago

Is it Object.keys itself that isn't present? Or is it failing when arg is undefined?

Souldrinker commented 8 years ago

It is failing when arg is null.

EisenbergEffect commented 8 years ago

@Souldrinker Can you check on the spec and see what it should do when Object.keys is called on null or undefined? Perhaps this is an area where IE supports the method but had an incorrect implementation. I'd like to know. If it is, we can detect for that scenario and overwrite IE's implementation.

Souldrinker commented 8 years ago

Well, as far as I can see IE does it correctly according to EcmaScript5: http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.14 (If the Type(O) is not Object, throw a TypeError exception.)

It says the same here: https://msdn.microsoft.com/en-us/library/ff688127(v=vs.94).aspx.

While it seems Chrome is doing it differently and accepting non-object arguments.

Could it be that Chrome uses the ES6 version of Object.keys that according to MDN differs (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys) "In ES5, if the argument to this method is not an object (a primitive), then it will cause a TypeError. In ES6, a non-object argument will be coerced to an object."

EisenbergEffect commented 8 years ago

Yep, that has to be it. We will need to check that behavior and update if it isn't present.

EisenbergEffect commented 8 years ago

Looking into the spec, if you pass null or undefined to Object.keys, it should throw an error. However, if you pass a primitive, it would previously throw an error, but should not in the newest spec. IE11 doesn't implement the latest version. Perhaps that is the issue?

EisenbergEffect commented 8 years ago

Brought up to spec. Releasing coming soon. However, bear in mind that the spec says that undefined and null should still throw. Primitives should not though.

stoffeastrom commented 8 years ago

So just to clarify. This could still throw an exception from the fetch client buildRequest since it's defaulting the bodyObj to null

 var bodyObj = body ? { body: body } : null;
    var parsedDefaultHeaders = parseHeaderValues(defaults.headers);
    var requestInit = Object.assign({}, defaults, { headers: {} }, source, bodyObj);
Souldrinker commented 8 years ago

Ok, thanks for looking into this. And sure, you probably fixed the issue where IE was using the ES5 implementation and now it supports primitives the ES6 way.

But unfortunately that doesn't help me getting rid of core-js in IE11, since I wasn't sending a primitive but null or undefined, so now with 1.0.3 I get a Cannot convert undefined or null to object instead. And I don't really know how I would be able to fix that.

It's not a big issue for me, since I can live with core-js, but if you would want Aurelia to work with IE11 and the fetch client you might want to dig further into this (unless it's me that does something wrong).

As I'm not really used to debugging in IE and it seems like it pretty much sucks I can't really follow what happens, but I'll try to explain as far as I've come.

The first call that seems to have caused the issue in IE was when I did a this.http.fetch("api/myurl"), that is I don't pass in the optional RequestInit as a second parameter to the fetch client's fetch function, but just sends an url.

In my main.ts I've set up default headers (so it works in Firefox which seems to default to xml rather than json) if that matters:

aurelia.container.registerInstance(HttpClient, new HttpClient().configure(builder => builder.withDefaults({ headers: { "Accept": "application/json", "Content-Type": "application/json"}, credentials: "same-origin"})));

For some reason I can't get IE to stop on my breakpoints to see what really is going on, but as far as I know the fetch stuff is being polyfilled using the whatwg-fetch stuff for IE and Safari (if that matters?) and as far as I can see Chrome relies on it's own implementation of Object.assign while IE that doesn't support ES6 uses your polyfill.

The arguments I then get down to your polyfill (not sure where they're being set) is: [0] "api/myurl" [1] undefined [2] [object] containing credentials="same-origin" and headers (application/json).

I don't really know what the second argument corresponds to and where it comes from, but in this case it causes IE to fail on O.keys since it according to spec should throw a TypeError.

I agree with you that Object.keys should throw an error according to spec when being fed an undefined or null, but for some reason Object.assign in core-js or Chrome works when using this.http.fetch("api/myurl"), while it seems like your polyfill in IE doesn't.

Maybe core-js/chrome removes or replaces these arguments before calling Object.keys (or doesn't use keys at all?)

mattgaspar commented 8 years ago

I've been trying to find the source of this issue as I encountered it today as well. Can't figure out how to debug in IE though! F12 tools stack trace doesn't show enough. Could the fact that systemjs automatically loads system-polyfills when it detects IE be causing some kind of conflict?

By the way, the aurelia docs site also doesn't load in IE11 currently

Potentially unhandled rejection [1] TypeError: Cannot convert undefined or null to object at Anonymous function (eval code:2:18657) at Anonymous function (eval code:2:19229) at b.prototype.activate (eval code:11:31762) at Anonymous function (eval code:11:31558) at $ (http://aurelia.io/jspm_packages/system-polyfills.js:4:8727) at H (http://aurelia.io/jspm_packages/system-polyfills.js:4:8314) at R.prototype.when (http://aurelia.io/jspm_packages/system-polyfills.js:4:12046) at b.prototype.run (http://aurelia.io/jspm_packages/system-polyfills.js:4:11080) at t.prototype._drain (http://aurelia.io/jspm_packages/system-polyfills.js:4:2985) at drain (http://aurelia.io/jspm_packages/system-polyfills.js:4:2652)

EisenbergEffect commented 8 years ago

I don't think it's system-polyfills. That error message is one I wrote myself in the last update (based on Chrome's standard message for the scenario). So, I do think it's probably coming from our Object.keys polyfill. However, I'd like to know what is calling that. Perhaps the bug is actually inside of the Object.assign polyfill?

EisenbergEffect commented 8 years ago

@Souldrinker @stoffeastrom Is it likely then that the bug is actually in the Object.assign polyfill? the issue being that it isn't handling null/undefined as one of its arguments? rather than an issue with Object.keys?

matthewma7 commented 8 years ago

Just throw in some input here. First, I personally don't know if the Document website and Object.Key are caused by the same issue.

For Object.Key. As Rob said, It really is related to Object.assign polyfill. From this.options = Object.assign({ routeHandler: this.loadUrl.bind(this) }, this.options, options); To O.keys(arg).concat(filterOS(arg)).forEach(set);, where the arg here is the culprit.

However, in my case the last parameter options is undefined in both IE 11 and Chrome. But Chrome native Object.assign didn't throw anything. (I am not saying which behavior is adhere to standard).

Souldrinker commented 8 years ago

@EisenbergEffect Yes, it sounds like that might be it. Looking at the MDN polyfill: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign, it seems like they do a null/undefined check of the arguments before adding it to the output.

I tried to understand how core-js did it (https://github.com/zloirock/core-js/blob/master/modules/_object-assign.js), but I had a bit of hard time trying to read that and I didn't find out how Chrome doesn't (at least not by a quick google).

But as stated in the previous post here it seems like both browsers gets the same input to Object.assign and Chrome/core-js can handle it and your polyfill (that kicks in using IE11) can't. So the arguments probably needs a null/undefined check.

But maybe the root cause of the issue is that aurelia-fetch-client passes in undefined values to start with? But since Chrome/core-js can handle it, it would probably be better to solve it in the Object.assign polyfill.

EisenbergEffect commented 8 years ago

We can fix our Object.assign polyfill. It's a quick and easy fix. We'll have it out today.

stoffeastrom commented 8 years ago

I thought we were talking about the Object.assign polyfill all along :). So yes the fix is to make sure to default to empty object here

mattgaspar commented 8 years ago

FYI - I tried editing the aurelia polyfill to return an empty array or an empty object instead of throwing an error on null and both produce this error:

Potentially unhandled rejection [1] TypeError: Object.getOwnPropertyNames: argument is not an Object at getOwnPropertySymbols (eval code:113:7) at Anonymous function (eval code:481:11) at assign (eval code:496:13) at activate (eval code:1513:7) at Anonymous function (eval code:1485:23) at $ (https://localhost/jspm_packages/system-polyfills.js:4:8727) at H (https://localhost/jspm_packages/system-polyfills.js:4:8314) at R.prototype.when (https://localhost/jspm_packages/system-polyfills.js:4:12046) at b.prototype.run (https://localhost/jspm_packages/system-polyfills.js:4:11080) at t.prototype._drain (https://localhost/jspm_packages/system-polyfills.js:4:2985)

By the way, if anyone wants a temporary fix, the code below from the aurelia docs works with IE11 (install "polymer/mutationobservers" from jspm first)

mattgaspar commented 8 years ago

Just noticed stoffeastrom was talking about a different location in the polyfill than where the error is thrown. It does work in IE11 when setting arg to an empty object here

if (arg === undefined || arg === null) arg = {};

EisenbergEffect commented 8 years ago

I can release a polyfill shortly. I think if it's null or undefined we'll just continue the loop.

EisenbergEffect commented 8 years ago

Released path for Object.assign with null/undefined check.

Souldrinker commented 8 years ago

Perfect, I tried removing core-js from the app now again and it still works fine in IE11 :-)

stoffeastrom commented 8 years ago

:+1: