-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing methods/globals in IE7, IE8 environments #2259
Conversation
CI failure seems unrelated to PR, anyone care to re-run it? |
Actually, the failure might be due to the PR; Node 0.8, and only Node 0.8, is complaining about the added "@segment/to-iso-string" dependency, reacting as though you'd specified a dependency named just "segment". It's hard to find documentation that far back to confirm it, but I'd hypothesize that scoped dependencies weren't supported in the version of NPM that comes with Node 0.8... |
Huh—I would think that this script would update npm before installing deps. That was the intention behind adding it, anyway. Not really sure why we try to install non-devDeps using the version of npm bundled with node 0.8; updating npm on that platform should be a non-issue. At least that's what we decided back when we Browserified (ref). @dasilvacontin probably has an opinion here. Also happy to swap the dep out with a non-namespaced one if someone can suggest one. I wasn't able to find any other |
It looks like #2236 was deliberately designed to check this. For what it's worth, recently I've seen this discussion about supporting 0.8 out of the box: |
We are currently supporting it out of the box – not sure if there's really a reason why users can't upgrade
That would be ideal. We can switch to the one you proposed when we bump the major version. Also, @boneskull, pretty sure these changes will be needed for the browser testing ticket. Otherwise you'll likely bump into the exception. |
"commander": "2.3.0", | ||
"debug": "2.2.0", | ||
"diff": "1.4.0", | ||
"escape-string-regexp": "1.0.2", | ||
"glob": "3.2.11", | ||
"growl": "1.9.2", | ||
"jade": "0.26.3", | ||
"json3": "^3.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Carret makes npm
cry.
Thanks for the PR, btw! |
We ended up getting |
Has Mocha ever supported IE7? |
I'm going to rebase this and see what happens. |
Oh, right, it's failing b/c of the caret. It looks like there was some implicit support for IE7 somewhere. |
Is it possible to test that we don't add I suppose the same thing could hypothetically go for other shims/polyfills too, although it seems like most of the others I've seen are just acting as function variables used instead of the native stuff and not standing in place of native global variables. Also, if this does work, we should try adding IE7 to the Sauce tests. For that matter, maybe we should see if it will go back all the way to 5 or 6? (Heh heh, do those still exist? Weird fact: the compatibility mode setting in IE11's developer console has modes for 10, 9, 8, 7 and 5. That's right, 5, which nobody even talks about anymore, but not 6.) |
One other question: Does "json3" just return the existing JSON global if it's available? If not, we may want to use |
@ScottFreeCode Apparently, it does: https://github.com/bestiejs/json3/blob/master/lib/json3.js#L43-L47 But it also checks if the native implementation is spec compliant and stuff. |
Alright, this is passing. I'm going to merge the branch. @ndhoule Thanks for getting this started! |
This patch fixes two incompatibilities in pre-ES5 environments:
JSON
global (IE7)Date#toISOString
method (IE7, IE8)