Skip to content
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

Selectively use Object.defineProperty if it is available. #303

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

Do not merge, as it missing #298.

@ScottFreeCode can you confirm this is working for you?

Fixes #301

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.

3 participants