Dep updates, update polyfill handling #2536
Conversation
|
@felipewmartins The PR build doesn't run against IE https://travis-ci.org/axios/axios/jobs/609776027 I think this is because there is a secret for SauceLabs that only runs on |
|
Rebased and included more dep updates since I first opened the PR ( |
|
@chinesedfan @yasuf maybe one of you can review? 🙏 |
avindra
changed the title
lib/utils.js
Outdated
| function isArray(val) { | ||
| return toString.call(val) === '[object Array]'; | ||
| } | ||
| var _toString = Object.prototype.toString; |
There was a problem hiding this comment.
Solid PR. The only tiny thing I can mention is keeping the old name toString.
There was a problem hiding this comment.
I think I did this to remove the /* global directive. I've reverted to the old name per your suggestion 👍
* handles webpack 1 -> 4 migration
assume `Promise` is available, or polyfilled by the consumer
String.protoype.trim has good coverage (including IE9) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/Trim Also, the http adapter already uses the native method.
in IE9. So lets remove the custom polyfill. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray also resolves a few lint issues
this approach avoids having to include a fully baked promise solution. note: the tests already rely on appspot.com to be up and running
|
@chinesedfan updated again, and reverted I was also able to remove this directive: It seems to be no longer needed with the new build baseline |
| <script> | ||
| if (typeof Promise !== "function") | ||
| document.write('<script src="//cdnjs.cloudflare.com/ajax/libs/es6-promise/4.1.1/es6-promise.min.js"><\/script>'); | ||
| </script> |
There was a problem hiding this comment.
Added a Cloudflare CDN polyfill for promise, which should fix the IE build breaking in continuous integration in master builds.
I recalled a question here. What's the difference between those two promises? And did you really test by IE?
There was a problem hiding this comment.
@chinesedfan sorry for the late reply. The Promise here is only really needed for running the unit tests in IE.
In my opinion, the unit tests can/should be moved to a brower-less testing approach, which would allow for the removal of any Promise polyfill and faster testing, etc.
There was a problem hiding this comment.
@chinesedfan I had some trouble getting the IE VM from Microsoft up and running. Maybe Github actions can help with the problem here. They explicitly support the Windows containers:
|
Not interested in this anymore, someone else can feel free to continue if you're interested. |
| @@ -226,7 +204,7 @@ function forEach(obj, fn) { | |||
| obj = [obj]; | |||
| } | |||
|
|
|||
| if (isArray(obj)) { | |||
| if (Array.isArray(obj)) { | |||
There was a problem hiding this comment.
Proof that Array.isArray is well supported and possibly more reliable since it doesn't depend on a string representation check:
Continuation of PR #2410. That PR was accepted, and reverted in #2479 only because there was an IE test that broke on
masterbranch. The IE builds, for some reason, don't run on PR builds, so nobody caught the issue during PR review.Added a Cloudflare CDN polyfill for promise, which should fix the IE build breaking in continuous integration in
masterbuilds.