Skip to content

Update Navigator type #700

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

Merged
merged 17 commits into from
Jun 20, 2019

Conversation

jeffryang24
Copy link
Contributor

Changes

@msftclas
Copy link

msftclas commented May 12, 2019

CLA assistant check
All CLA requirements met.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

There are a few MS-specific interfaces that are removed in this PR. Can you add them back manually unless you are sure that nobody is using them?

@jeffryang24
Copy link
Contributor Author

jeffryang24 commented Jun 13, 2019

There are a few MS-specific interfaces that are removed in this PR. Can you add them back manually unless you are sure that nobody is using them?

I will try to add these specific interfaces back to the files. Sorry for my late response and thanks for notifying it... 👍

@jeffryang24
Copy link
Contributor Author

hmm... why the test still failed while my local success...? 🤔

@sandersn
Copy link
Member

It's because dom.generated.d.ts is generated from various widl files plus some overriding jsons. You need to add MSFileSaver and MSNavigatorDoNotTracktoinputfiles/addedTypes.jsonand then add them toNavigatorininputfiles/overridingTypes.json`.

Here's an example of the first:

    "interfaces": {
        "interface": {
            "MSFileSaver": {
                "name": "MSFileSaver",
                "methods": {
                    "method": {
                        "msSaveBlob": {
                            "name": "msSaveBlob",
                            "override-signatures": [
                                "msSaveBlob(blob: any, defaultName?: string): boolean"
                            ]
                        }
// more methods here ...
                    }
                },
                "no-interface-object": 1
            },

Here's what I've got in overridingTypes.json for Navigator, but it's not working properly yet:

        "interface": {
            "Navigator": {
                "name": "Navigator",
                "extends": ["MSFileSaver", "MSNavigatorDoNotTrack"]
            },

I'll keep looking at the code and update when I figure it out. @saschanaz any suggestions?

@saschanaz
Copy link
Contributor

You don't need to manually rewrite MSFileSaver, just do implements: ["MSFileSaver", "MSNavigatorDoNotTrack"] and it should automatically augment to the extends list and reuse existing types.

@jeffryang24
Copy link
Contributor Author

You don't need to manually rewrite MSFileSaver, just do implements: ["MSFileSaver", "MSNavigatorDoNotTrack"] and it should automatically augment to the extends list and reuse existing types.

Hmm... what's different between extends and implements? For example below section:

"SVGFEDropShadowElement": {
                "name": "SVGFEDropShadowElement",
                "exposed": "Window",
                "properties": {
                    "property": {
                        "in1": {
                            "name": "in1",
                            "read-only": 1,
                            "override-type": "SVGAnimatedString"
                        },
                        "dx": {
                            "name": "dx",
                            "read-only": 1,
                            "override-type": "SVGAnimatedNumber"
                        },
                        "dy": {
                            "name": "dy",
                            "read-only": 1,
                            "override-type": "SVGAnimatedNumber"
                        },
                        "stdDeviationX": {
                            "name": "stdDeviationX",
                            "read-only": 1,
                            "override-type": "SVGAnimatedNumber"
                        },
                        "stdDeviationY": {
                            "name": "stdDeviationY",
                            "read-only": 1,
                            "override-type": "SVGAnimatedNumber"
                        }
                    }
                },
                "methods": {
                    "method": {
                        "setStdDeviation": {
                            "name": "setStdDeviation",
                            "override-signatures": [
                                "setStdDeviation(stdDeviationX: number, stdDeviationY: number): void"
                            ]
                        }
                    }
                },
                "extends": "SVGElement",
                "implements": [
                    "SVGFilterPrimitiveStandardAttributes"
                ]
            },

The output is: interface SVGFEDropShadowElement extends SVGElement, SVGFilterPrimitiveStandardAttributes.

@saschanaz
Copy link
Contributor

saschanaz commented Jun 15, 2019

The extends in WebIDL means it "inherits" from the target interface: SVGFEDropShadowElement.__proto__.prototype === SVGElement.prototype, while implements means it just includes their members (meaning they are mixins). It's really a WebIDL thing rather than TypeScript.

@jeffryang24
Copy link
Contributor Author

Hmm... My local test already success, but why it failed again..? 🤔

@saschanaz
Copy link
Contributor

Hmm, it seems you should do rebase or merge from master branch.

@jeffryang24
Copy link
Contributor Author

Hmm, it seems you should do rebase or merge from master branch.

Ok, will merge from master first...

@jeffryang24
Copy link
Contributor Author

Currently already up to date with master.

@jeffryang24 jeffryang24 force-pushed the fix-mimetypearray-interface branch from 081d8d2 to e61b35f Compare June 15, 2019 06:35
@@ -2207,6 +2207,150 @@
}
}
}
},
"MSFileSaver": {
Copy link
Contributor

@saschanaz saschanaz Jun 15, 2019

Choose a reason for hiding this comment

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

All changes here in addedTypes.json look redundant. implements in overridingTypes.json should do these things automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the added interfaces in addedTypes? such as MSFileSaver, MSNavigatorDoNotTrack, ExceptionInformation, etc.?

Copy link
Contributor

@saschanaz saschanaz Jun 15, 2019

Choose a reason for hiding this comment

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

Yup. They were once removed from the result because implements did not included them (so the code thought they were needless), but as you added them again via overridingTypes.json, now those should be kept intact without these manual typings.

Copy link
Contributor Author

@jeffryang24 jeffryang24 Jun 15, 2019

Choose a reason for hiding this comment

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

But, some interfaces like MSLaunchUriCallback, ExceptionInformation, ConfirmSiteSpecificExceptionsInformation, StoreExceptionsInformation, and StoreSiteSpecificExceptionsInformation have been deleted, so I added it back manually through addedTypes.json.

Copy link
Contributor

@saschanaz saschanaz Jun 15, 2019

Choose a reason for hiding this comment

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

That's because we have a type tracker that auto-deletes unreferenced/unexposed dictionaries and interfaces. Try removing them and building again, and you'll see those types won't be removed again 😁

@saschanaz
Copy link
Contributor

The test failure seems to be from an ordering problem... What environment are you on? OS and node.js version, etc.

@@ -2207,6 +2207,20 @@
}
}
}
},
"MSLaunchUriCallback": {
"name": "MSLaunchUriCallback",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is a problem. Could you remove this and add MSLaunchUriCallback to "Window" field of knownTypes.json?

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, sure.. will mve it to knownTypes.json...

@jeffryang24
Copy link
Contributor Author

The test failure seems to be from an ordering problem... What environment are you on? OS and node.js version, etc.

OS: Office Notebook => Ubuntu 18.04 LTS (amd64), Personal Notebook (current): Manjaro Illyria 18.0.4 (amd64)
NodeJS: Office Notebook => lts/carbon (v8.15.1), Personal Notebook (current): lts/dubnium (v10.16.0)

@saschanaz
Copy link
Contributor

I'm not sure what's the core problem yet but confirmed that we have a compatibility problem with Node.js 8/10. Updating to Node.js 12 fixes the problem.

@jeffryang24
Copy link
Contributor Author

I'm not sure what's the core problem yet but confirmed that we have a compatibility problem with Node.js 8/10. Updating to Node.js 12 fixes the problem.

I see... will switch node to v12... 👍

@@ -10805,30 +10817,21 @@ declare var NavigationPreloadManager: {
};

/** The state and the identity of the user agent. It allows scripts to query it and to register themselves to carry on some activities. */
interface Navigator extends NavigatorID, NavigatorOnLine, NavigatorContentUtils, NavigatorStorageUtils, MSNavigatorDoNotTrack, MSFileSaver, NavigatorBeacon, NavigatorConcurrentHardware, NavigatorUserMedia, NavigatorLanguage, NavigatorStorage, NavigatorAutomationInformation {
readonly activeVRDisplays: ReadonlyArray<VRDisplay>;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should restore activeVRDisplays/getVRDisplays() and NavigatorBeacon. Looks good otherwise.

@saschanaz saschanaz mentioned this pull request Jun 15, 2019
@sandersn sandersn merged commit 7f5ab3c into microsoft:master Jun 20, 2019
@jeffryang24 jeffryang24 deleted the fix-mimetypearray-interface branch June 21, 2019 05:52
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.

Possible typo for MimeTypeArray interface
4 participants