Skip to content

General type updates#684

Merged
sandersn merged 3 commits intomicrosoft:masterfrom
saschanaz:namesakes
Jun 6, 2019
Merged

General type updates#684
sandersn merged 3 commits intomicrosoft:masterfrom
saschanaz:namesakes

Conversation

@saschanaz
Copy link
Copy Markdown
Collaborator

@saschanaz saschanaz commented Apr 12, 2019

Adds WebGLRenderingContextBase.canvas and self.requestAnimationFrame() for worker, and exposes External global constructor.

Had to modify the code to process the special same-named properties canvas. Not super pretty but should be good enough for now.

@saschanaz saschanaz mentioned this pull request Apr 24, 2019
@sandersn sandersn self-assigned this Jun 4, 2019
Copy link
Copy Markdown
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.

Questions about the namesakes code. Also, would a name like duplicateProperties or aliasedProperties be more precise?

},
"Performance": {
"name": "Performance",
"properties": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is just a bugfix right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That was my intention but looks like this is completely redundant. I'll remove this.

Comment thread src/widlprocess.ts
addComments(prop, commentMap, i.name, member.name);

if (member.name in properties!.namesakes!) {
properties!.namesakes![member.name].push(prop);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it looks like this can happen more than once, but the code in mergeNamesakes assumes there's only one entry in each namesakes in line 20: for (const [prop] of .... Is that correct?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yup, because in line 20 the namesakes value is already filtered by exposure ([Exposed] in IDL).

@saschanaz
Copy link
Copy Markdown
Collaborator Author

saschanaz commented Jun 5, 2019

Questions about the namesakes code. Also, would a name like duplicateProperties or aliasedProperties be more precise?

It's not precisely "duplicated" or "aliased" because those properties are mutually exclusive (only one property should be alive and others will be filtered out, for each lib target):

interface X {
  [Exposed=Window] attribute Foo x; // This one will be used in dom.generated.d.ts
  [Exposed=Worker] attribute Bar x; // This one will be used in worker.generated.d.ts
};

"duplicate" makes me think those are same properties but they are not really same, so 🤔

Copy link
Copy Markdown
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.

sounds like evilTwin might be the most accurate name :). Namesake is a fine name for now, in any case.

@sandersn sandersn merged commit ab3d46e into microsoft:master Jun 6, 2019
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.

2 participants