Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

fix: IE =< 8 incompatibility regression #43

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

lwr
Copy link
Contributor

@lwr lwr commented Sep 28, 2017

0.7.1 breaks IE8, this commit takes it back

Notabel Changes

  1. IE8 does not have console.error
  2. eval.call(null, src) does not work for IE8, the eval code is limited to local scope, not global

Issues

Related #41

@jsf-clabot
Copy link

jsf-clabot commented Sep 28, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

lwr referenced this pull request Sep 28, 2017
addScript.js Outdated
eval.call(null, src);
} else if (typeof execScript !== "undefined") {
if (typeof execScript !== "undefined"
/* "bad" IE */ && typeof attachEvent !== "undefined" && typeof addEventListener === "undefined") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extract the IE Check into its own block/helper

// Check for IE =< 8
function isIE () {
  return typeof ...
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the helper above L5

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done

addScript.js Outdated
report(error);
}

function report(error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

report => log

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the helper above L5

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lwr Thx

addScript.js Outdated
@@ -3,22 +3,27 @@
Author Tobias Koppers @sokra
*/
module.exports = function(src) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final nitpick 🐤 No block padding here please 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, squashed

0.7.1 breaks IE8, this commit takes it back
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants