Skip to content

Conversation

@mcollina
Copy link
Member

Do not merge, as it missing #298.

@ScottFreeCode can you confirm this is working for you?

Fixes #301

@mcollina mcollina requested a review from calvinmetcalf June 27, 2017 13:46
Copy link
Contributor

@calvinmetcalf calvinmetcalf left a comment

Choose a reason for hiding this comment

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

good if green

@mcollina
Copy link
Member Author

I will update when 8.1.3 comes out.

@ScottFreeCode
Copy link

Tried pulling this in by using "nodejs/readable-stream#maybe-ie7-ie8" as "readable-stream"; seems to have fixed IE 7, but we've still got some issue with IE 8 and it's not showing the line number from the original file (or even which file was the original); let me talk to the team lead about getting access to some kind of cache or artifact upload of the browser bundle so we can try to determine what that line is...

/* <replacement> */
var maybeDefineProperty = Object.defineProperty;
// IE < 9
if (!maybeDefineProperty || global.document && global.document.documentMode < 9) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ScottFreeCode you might be more familiar with IE 8 than me, can you confirm this is the correct way to detect if we are running on IE8? I need to disable it even though it is present.

Choose a reason for hiding this comment

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

Looks right as far as I'm aware; document.documentMode is 11 on the last IE unless I fiddle with the compatibility mode, in which case it becomes 8 on compatibility mode for IE 8. Moreover, the fix did seem to work for IE7, which is even lower.

Running this bit of code (the if condition with !maybeDefineProperty replaced with !Object.defineProperty) in compatibility mode for 7, 8 and 9 got true for 7 and 8 and false for 9.

There's always brute-force try-catch-based feature sniffing to deal with the fact that IE 8 has the function but it doesn't work right; but that would probably be overkill considering this works fine as far as I can tell.

Copy link
Member Author

Choose a reason for hiding this comment

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

the problem is that it is not working as expected. The error you are seeing is due to IE 8 broken Object.defineProperty. Something in that bit of logic is wrong. I would not like to use the try-catch approach.

@mcollina mcollina mentioned this pull request Jun 29, 2017
@mcollina mcollina closed this Jun 29, 2017
@mcollina mcollina reopened this Jun 29, 2017
@mcollina mcollina closed this Jun 29, 2017
@mcollina mcollina deleted the maybe-ie7-ie8 branch June 29, 2017 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants