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

Classes should have constructors whenever possible #80

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@domenic
Member

domenic commented Oct 7, 2017

The long-awaited text on why constructors are good.

create instances of the class for their own purposes, such as testing, mocking, or interfacing with
third-party libraries which accept instances of that class. Additionally, because of how
JavaScript's class design works, it is only possible to create a subclass of the class if it is
constructible.

This comment has been minimized.

@marcoscaceres

marcoscaceres Oct 9, 2017

Member

Because a class can be so easily defined in IDL, and because those classes end up in all global scopes, there needs to be strong advice to really generalize classes for maximum reuse (i.e., WG should talk to the community!)... and that "data only" classes, should just use Dictionaries (i.e., objects). Payment Request has the worst of both, for example: PaymentAddress is non-constructible, and pure data... it's tragic :(

@marcoscaceres

marcoscaceres Oct 9, 2017

Member

Because a class can be so easily defined in IDL, and because those classes end up in all global scopes, there needs to be strong advice to really generalize classes for maximum reuse (i.e., WG should talk to the community!)... and that "data only" classes, should just use Dictionaries (i.e., objects). Payment Request has the worst of both, for example: PaymentAddress is non-constructible, and pure data... it's tragic :(

This comment has been minimized.

@annevk

annevk Oct 9, 2017

Member

You should still be able to fix that, no?

@annevk

annevk Oct 9, 2017

Member

You should still be able to fix that, no?

This comment has been minimized.

@marcoscaceres

marcoscaceres Oct 9, 2017

Member

If you mean adding a Constructor on these interfaces, then yes. We should probably do that -as it would not break anything, I think.

@marcoscaceres

marcoscaceres Oct 9, 2017

Member

If you mean adding a Constructor on these interfaces, then yes. We should probably do that -as it would not break anything, I think.

This comment has been minimized.

@annevk

annevk Oct 9, 2017

Member

I meant turning them into a dictionary if they're just data.

@annevk

annevk Oct 9, 2017

Member

I meant turning them into a dictionary if they're just data.

This comment has been minimized.

@marcoscaceres

marcoscaceres Oct 9, 2017

Member

No, unfortunately. Couple of reasons:

  1. It would be a significant breaking change - as, IIRC, IDL attributes can't return Dictionaries, the API would need some sort of .getAddress() method - right now the PaymentAddress is returned via an attribute.
  2. The interface on which this attribute is exposed has .toJSON(), which itself calls .toJSON() on the attribute (returning a nested JSON structure).
  3. Less importantly, but something that came up, there are two implementations that have been shipping for about 1 year - so implementers were reluctant to change things in a way that was not backwards compatible.
@marcoscaceres

marcoscaceres Oct 9, 2017

Member

No, unfortunately. Couple of reasons:

  1. It would be a significant breaking change - as, IIRC, IDL attributes can't return Dictionaries, the API would need some sort of .getAddress() method - right now the PaymentAddress is returned via an attribute.
  2. The interface on which this attribute is exposed has .toJSON(), which itself calls .toJSON() on the attribute (returning a nested JSON structure).
  3. Less importantly, but something that came up, there are two implementations that have been shipping for about 1 year - so implementers were reluctant to change things in a way that was not backwards compatible.

This comment has been minimized.

@ojanvafai

ojanvafai Apr 9, 2018

PaymentAddress does seem like a strange mix of dictionaries and classes.

However, as discussed elsewhere I don't think we should be returning dictionaries from APIs in general as it makes it impossible to add methods to them in the future. Using dictionaries as arguments to functions as a way of having optional values sounds fine though.

This might actually be a thing the TAG should have a design principle by itself. @slightlyoff @domenic

@ojanvafai

ojanvafai Apr 9, 2018

PaymentAddress does seem like a strange mix of dictionaries and classes.

However, as discussed elsewhere I don't think we should be returning dictionaries from APIs in general as it makes it impossible to add methods to them in the future. Using dictionaries as arguments to functions as a way of having optional values sounds fine though.

This might actually be a thing the TAG should have a design principle by itself. @slightlyoff @domenic

This comment has been minimized.

@domenic

domenic Apr 9, 2018

Member

It doesn't make it impossible; it just means you have to upgrade to a class at that time.

@domenic

domenic Apr 9, 2018

Member

It doesn't make it impossible; it just means you have to upgrade to a class at that time.

<h3 id="constructors">Classes should have constructors when possible</h3>
By default, [[!WEBIDL]] interfaces are reified in JavaScript as "non-constructible" classes: trying

This comment has been minimized.

@marcoscaceres

marcoscaceres Oct 9, 2017

Member

nit: "reified" seems like fancy jargon (specially because it conjures up "semantic web reification" 🤢). Is there a simpler term we can use here? If no, that's ok.

@marcoscaceres

marcoscaceres Oct 9, 2017

Member

nit: "reified" seems like fancy jargon (specially because it conjures up "semantic web reification" 🤢). Is there a simpler term we can use here? If no, that's ok.

This comment has been minimized.

@annevk

annevk Oct 9, 2017

Member

represented?

@annevk

annevk Oct 9, 2017

Member

represented?

This point likely needs further discussion,
and perhaps further experience of use of these types.
It's also worth considering the implications of having

This comment has been minimized.

@ojanvafai

ojanvafai Apr 9, 2018

IMO, this is the primary reason to pushback on live collections. It's not the global performance cost though, it's that the performance cost is action at a distance, e.g. holding on to NodeLists makes completely unrelated methods, e.g. appendChild, slow in a way that's non-obvious.

@ojanvafai

ojanvafai Apr 9, 2018

IMO, this is the primary reason to pushback on live collections. It's not the global performance cost though, it's that the performance cost is action at a distance, e.g. holding on to NodeLists makes completely unrelated methods, e.g. appendChild, slow in a way that's non-obvious.

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