paulmillr / es6-shim

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

Fix for IE11 check before `preserveToString` #359

Closed AlastairTaft closed 8 years ago

AlastairTaft commented 8 years ago

When in IE11 the 'preserve string' method crashes on startsWith. startsWith doesn't exist in IE11 so the preserveToString method doesn't work (there's nothing to preserve).

Added a check to prevent it running if it doesn't need to and it's going to cause an error.

ljharb commented 8 years ago

Thanks for the report!

I'd rather fix this in the place overrideNative is being called on a non-present native. It seems like if this checked that startsWith and endsWith both exist, before entering the block, it might solve it also?

That said, which version of IE 11 are you using? In my copy, this does not error out.

AlastairTaft commented 8 years ago

In the overrideNative method you might be relying on the bit of code defineProperty(object, property, replacement, true); to set and make the startsWith property exist, but I'm not sure with this.

I think the startsWithIsCompliant is being set to undefined because it looks like both methods startsWithRejectsRegex and startsWithHandlesInfinity are probably returning undefined because they have checks like this at the start return String.prototype.startsWith && .... So the setting of the value will be startsWithRejectsRegex() && startsWithHandlesInfinity will be undefined && undefined.

My IE version says Version: 11.0.9600.17937 UpdateVersion:11.0.22

ljharb commented 8 years ago

I think you're totally correct that I'm incorrectly calling overrideNative in this case - I'm just surprised it hasn't failed before.

If you're open to editing and rebasing your PR so that it avoids https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L735-L739 entirely when startsWith or endsWith aren't present, I'd be happy to merge that! As is, I'd prefer overrideNative to blow up when passed a nonexistent native, since it indicates that I did something wrong :-)

ljharb commented 8 years ago

Awesome! If you can fix these comments, and then rebase it down to a single commit on top of the latest master, this will be good to go!

ljharb commented 8 years ago

Please be sure to rebase this - merge commits make git logs sad

AlastairTaft commented 8 years ago

My git skills are definitely lacking because I can't seem to squash these commits.

ljharb commented 8 years ago

@AlastairTaft git fetch ; git rebase -i origin/master and edit the messages according to the instructions, and when you're done, git push -f

ljharb commented 8 years ago

@AlastairTaft Thanks for reporting this! I fixed this myself, since it's pretty important and I couldn't wait any longer for your rebase. I appreciate the effort!

AlastairTaft commented 8 years ago

Sorry, this dropped off my radar.