-
Notifications
You must be signed in to change notification settings - Fork 230
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
Conversation
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.
good if green
I will update when 8.1.3 comes out. |
Tried pulling this in by using |
/* <replacement> */ | ||
var maybeDefineProperty = Object.defineProperty; | ||
// IE < 9 | ||
if (!maybeDefineProperty || global.document && global.document.documentMode < 9) { |
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.
@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.
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.
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.
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.
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.
Do not merge, as it missing #298.
@ScottFreeCode can you confirm this is working for you?
Fixes #301