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

lib: make DOMException attributes configurable and enumerable #22550

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

The name, message and code attributes of the DOMException
interface should be enumerable and configurable. Aligning
the definition with the Web allows us to use it when
running the Web Platform Tests.

Refs: https://heycam.github.io/webidl/#idl-DOMException
Refs: web-platform-tests/wpt@125950d

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

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.

Thanks for this – not sure how I overlooked it when creating the class. I have become a fan of the way TextEncoder sets the property descriptor bits though, as something like this:

class DOMException ... {
  get name() {
    // …
  }
  get message() {
    // …
  }
}
Object.defineProperties(DOMException.prototype, {
  name: { enumerable: true },
  message: { enumerable: true }
});

This has several advantages, the first being that we can continue to use the nice class syntax, and the second being that Object.getOwnPropertyDescriptor(DOMException.prototype, 'name').get.name would be 'get name' rather than 'get', which is what the IDL spec mandates (see step 3).

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

couldn't we also use defineIDLClass

@joyeecheung
Copy link
Member Author

I have become a fan of the way TextEncoder sets the property descriptor bits though, as something like this:

@TimothyGu Indeed that looks nicer! Thanks for the suggestion, I'll switch to that.

couldn't we also use defineIDLClass

@devsnek defineIDLClass doesn't seem to deal with attributes ATM..we may want to split that into a separate file dedicated to IDL bindings and add support for attributes later.

@jdalton
Copy link
Member

jdalton commented Aug 28, 2018

I have mixed feels about Node throwing DOM exceptions.

Update:

I mean if we're doing it, then it should be correct, but I can see this being slippery.

Copy link
Member

@jdalton jdalton left a comment

Choose a reason for hiding this comment

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

By the IDL shouldn't DOMException be exposed on the global and worker context?

@@ -43,6 +43,12 @@ class DOMException extends Error {
}
}

Object.defineProperties(DOMException.prototype, {
name: { enumerable: true, configurable: true },
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the configurable: true isn't necessary, as the property is already configurable and Object.defineProperties won't override it.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to be explicit here since Object.defineProperties will read inherited properties of descriptors and Node internals aren't run in an isolated realm yet.

Copy link
Member

Choose a reason for hiding this comment

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

DOMException was created right above this with its prototype having them as own properties, so that is not an issue here.

@TimothyGu
Copy link
Member

@jdalton, it was a conscious decision when the class was added to not expose it globally, precisely because of the "slippery slope" you mentioned. It is instead only used where it's necessary for compatibility with HTML's MessagePort. In any case, even if we were to do such a change it should be a separate PR, consistent with the tradition of making Web-derived classes globals in their own PRs.

@jdalton
Copy link
Member

jdalton commented Aug 28, 2018

By the IDL does DOMException error have a .code property?

Ah, it does 👍

Update:

Other thought: I'm not sure if it's implied but at least in Chrome the DOMException.prototype has a Symbol.toStringTag value of "DOMException".

@TimothyGu
Copy link
Member

Other thought: I'm not sure if it's implied but at least in Chrome the DOMException.prototype has a Symbol.toStringTag value of "DOMException".

Oh yeah, this probably needs

  [Symbol.toStringTag]: { configurable: true, value: 'DOMException' }

@jdalton Would you be open to approving this PR once that's fixed?

@jdalton
Copy link
Member

jdalton commented Aug 28, 2018

Would you be open to approving this PR once that's fixed?

Yep, this is already out of the bottle. The main reason I held is because it had more than 2 approvals, meaning a merge could be imminent, and I wanted a bit more discussion.

@joyeecheung
Copy link
Member Author

Yep, this is already out of the bottle. The main reason I held is because it had more than 2 approvals, meaning a merge could be imminent, and I wanted a bit more discussion.

I don't quite understand...do you want more discussion about the enumerability of these existing properties? If not, in general we open new issues or PRs to discuss, instead of trying to fix everything in one PR.

@jdalton
Copy link
Member

jdalton commented Aug 29, 2018

Following up, I dug up spec bits I think are relevant for the Symbol.toStringTag value.

The Symbol.toStringTag addition spec'd for DOMException here:


and

and

While it doesn't state having Symbol.toStringTag on the prototype we could follow Chrome's lead (and it matches how it's handled in other builtins).

@joyeecheung
Copy link
Member Author

@jdalton Can you open another PR for the Symbol.toStringTag changes? Maybe also with

By the IDL shouldn't DOMException be exposed on the global and worker context?

that you requested? I don't see why those have to be fixed in this particular PR and block this one?

@jdalton
Copy link
Member

jdalton commented Aug 30, 2018

My requested change is in line with "align DOMException with the Web" and is just an additional line after the 3 properties you've defined.

@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 30, 2018

My requested change is in line with "align DOMException with the Web" and is just an additional line after the 3 properties you've defined.

Does it help if I change the commit title to "lib: make DOMException attributes configurable and enumerable"?

I don't mind fixing those myself, but personally even if I fix them in this PR I would not squash them into the first commit when I land the changes, so I'd prefer to have another PR open for that (In general I prefer to squash every commit in one PR whenever possible and keep commits that should be separated in different PR).

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM (no matter if we add the toStringTag in this PR or not and no strong opinion about explicitly adding configurable: true)

The `name`, `message` and `code` attributes of the DOMException
interface should be enumerable and configurable. Aligning
the definition with the Web allows us to use it when
running the Web Platform Tests.

Refs: https://heycam.github.io/webidl/#idl-DOMException
@joyeecheung joyeecheung changed the title lib: align DOMException with the Web lib: make DOMException attributes configurable and enumerable Sep 16, 2018
@joyeecheung
Copy link
Member Author

Rebased and renamed the PR to focus on the enumerability and configurability. I think other compliance issues can be dealt with in another PR - I'll open one next week if no one picks it up.

CI: https://ci.nodejs.org/job/node-test-pull-request/17211/

@joyeecheung
Copy link
Member Author

@jdalton Do you insist that this PR fixing the enumerability and configurability cannot land until other attributes get fixed?

@jdalton
Copy link
Member

jdalton commented Sep 16, 2018

@joyeecheung Yes please. It's a single additional attribute and I believe a reasonable change request.

@joyeecheung
Copy link
Member Author

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Sep 18, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Sep 18, 2018

Per @jdalton 's request this is blocked by #22933 (though I still don't quite understand the reasoning behind this PR fixing the enumerability and configurability cannot land until other attributes get fixed)

@joyeecheung
Copy link
Member Author

CI is green

joyeecheung added a commit that referenced this pull request Sep 20, 2018
PR-URL: #22933
Refs: #22550
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in 676bd5f, thanks!

joyeecheung added a commit that referenced this pull request Sep 20, 2018
The `name`, `message` and `code` attributes of the DOMException
interface should be enumerable and configurable. Aligning
the definition with the Web allows us to use it when
running the Web Platform Tests.

Refs: https://heycam.github.io/webidl/#idl-DOMException

PR-URL: #22550
Refs: web-platform-tests/wpt@125950d
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 21, 2018
PR-URL: #22933
Refs: #22550
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 21, 2018
The `name`, `message` and `code` attributes of the DOMException
interface should be enumerable and configurable. Aligning
the definition with the Web allows us to use it when
running the Web Platform Tests.

Refs: https://heycam.github.io/webidl/#idl-DOMException

PR-URL: #22550
Refs: web-platform-tests/wpt@125950d
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants