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

fix: Add [ImplicitThis] behaviour #166

Merged
merged 4 commits into from
Mar 16, 2020

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Feb 13, 2020

This implements the [ImplicitThis] behaviour present on all WebIDL attributes and operations.

Needed for jsdom/jsdom#2835
Will allow fixing jsdom/jsdom#2830

See whatwg/webidl#155 for details.

@ExE-Boss ExE-Boss force-pushed the feat/global/implicit-this branch 2 times, most recently from b8ee8ef to fdb0c75 Compare February 14, 2020 01:28
@ExE-Boss ExE-Boss force-pushed the feat/global/implicit-this branch from 778968e to 1aed7f7 Compare February 14, 2020 02:01
@ExE-Boss ExE-Boss changed the title feat(Global): Add [ImplicitThis] behaviour fix: Add [ImplicitThis] behaviour Feb 15, 2020
@domenic
Copy link
Member

domenic commented Feb 16, 2020

[ImplicitThis]

I think this phrasing is confusing since there is no extended attribute [ImplicitThis].

Fixes jsdom/jsdom#2830

This is not correct, right? I.e. merging this would not fix that bug.


As for the approach, I wish there were a way to generate this only for the cases where it matters, i.e. functions on the prototype chain of the global object. From what I understand that's more of how browser implementations do it. But this works I guess, and the generated code isn't too heavyweight. And there is some value in following the spec as close as possible. So I'm inclined to go with this approach, although it'd be good to get other maintainers' thoughts.

@ExE-Boss
Copy link
Contributor Author

This is how whatwg/webidl#155 is specified to behave.

Case in point: https://heycam.github.io/webidl/#dfn-create-operation-function

@domenic
Copy link
Member

domenic commented Feb 16, 2020

Yes, as I said above,

I wish there were a way to generate this only for the cases where it matters, i.e. functions on the prototype chain of the global object. From what I understand that's more of how browser implementations do it. But this works I guess, and the generated code isn't too heavyweight. And there is some value in following the spec as close as possible

@ExE-Boss
Copy link
Contributor Author

Right, I somehow missed the paragraph after the horizontal line.

As for jsdom/jsdom#2830, that will be fully fixed once this is released and jsdom/jsdom#2835 is merged.

@domenic
Copy link
Member

domenic commented Mar 14, 2020

I guess we should be sure not to merge this until Window.js has appropriate brand checks on all its methods; otherwise it would be a regression.

@domenic
Copy link
Member

domenic commented Mar 14, 2020

Wait, no, that's wrong, I was confused.

Is there any reason to hold off on merging this? I guess maybe because it adds to the output code size slightly for no observable gain at this point...

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Mar 14, 2020

It removes the need to use the this.addEventListener = this.addEventListener.bind(this) hack in Window.js:

///// METHODS for [ImplicitThis] hack
// See https://lists.w3.org/Archives/Public/public-script-coord/2015JanMar/0109.html
this.addEventListener = this.addEventListener.bind(this);
this.removeEventListener = this.removeEventListener.bind(this);
this.dispatchEvent = this.dispatchEvent.bind(this);

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Looks good. One question.

let objName = `this`;
if (onInstance) { // we're in a setup method
objName = `obj`;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you tell me if my understanding is correct: this isn't needed because for unforgeable methods/properties this always points to obj anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, yes, unless you were to use unforgeableMethod.call, in which case this will be whatever was passed to call as the thisArg parameter.

Comment on lines 120 to +121
addMethod("toString", [], `
if (!this || !exports.is(this)) {
const esValue = this;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TimothyGu TimothyGu merged commit 2a2a099 into jsdom:master Mar 16, 2020
@TimothyGu TimothyGu deleted the feat/global/implicit-this branch March 16, 2020 07:45
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