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

[IE8] Fix in 1.1.5 only handles 2 levels of dynamically nested classes #50 #51

Merged
merged 1 commit into from
May 4, 2022

Conversation

MSNev
Copy link
Collaborator

@MSNev MSNev commented May 3, 2022

No description provided.

@MSNev MSNev added this to the 1.1.6 milestone May 3, 2022
if (!target[DynProtoPolyProto]) {
target[DynProtoPolyProto] = newProto;
}
let curProto = target[str__Proto] || target[Prototype] || (target[Constructor] ? target[Constructor][Prototype] : null);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: This code is only reached for runtimes that don't support getPrototypeOf() https://caniuse.com/?search=getPrototypeOf

// As this prototype doesn't have this property then this is from an inherited class so newProto is the base to return so save it
// so we can look it up value (which for a multiple hierarchy dynamicProto will be the base class)
newProto = target[DynProtoBaseProto] = target[DynProtoCurrent] || target[DynProtoBaseProto];
target[DynProtoCurrent] = curProto;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For IE, we need to track both the current and previous as once we are looking at a sub-class we can't see the parent.

if (!_hasOwnProperty(target, DynProtoBaseProto)) {
// As this prototype doesn't have this property then this is from an inherited class so newProto is the base to return so save it
// so we can look it up value (which for a multiple hierarchy dynamicProto will be the base class)
newProto = target[DynProtoBaseProto] = target[DynProtoCurrent] || target[DynProtoBaseProto];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might look strange, but the trick here is that because the previous prototype has a DynProtoCurrent value of that prototype is what we get (before we assign the current one on the next line).

@@ -161,9 +173,8 @@ function _getObjProto(target:any) {
*/
function _forEachProp(target: any, func: (name: string) => void) {
let props: string[] = [];
let getOwnProps = Obj["getOwnPropertyNames"];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just moved this so it's only done once instead of every call as it won't change.

@MSNev MSNev merged commit aaae362 into master May 4, 2022
@@ -133,25 +143,28 @@ function _isObjectArrayOrFunctionPrototype(target:any)
* @ignore
*/
function _getObjProto(target:any) {
let newProto;

if (target) {
// This method doesn't existing in older browsers (e.g. IE8)
Copy link
Member

Choose a reason for hiding this comment

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

exist

Copy link
Member

@ramthi ramthi left a comment

Choose a reason for hiding this comment

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

I think it looks good. What type of test coverage do we have for this code?

@MSNev
Copy link
Collaborator Author

MSNev commented May 4, 2022

Manual only as this path is only executed on older browsers, so I created a couple of manual "HelloWorld.html" files for ApplicationInsights and 1P version, which I can then enable IE mode and force it into IE8 mode. Which requires manually debugging and stepping into this area of the code.

Which isn't fun because of the amount of debugging information available with IE or Edge in IE Mode is well, bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants