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

input.valueAsDate violate "obj.attribute === obj.attribute must always hold" #139

Closed
hax opened this issue Sep 30, 2019 · 8 comments · Fixed by #165
Closed

input.valueAsDate violate "obj.attribute === obj.attribute must always hold" #139

hax opened this issue Sep 30, 2019 · 8 comments · Fixed by #165
Assignees

Comments

@hax
Copy link

hax commented Sep 30, 2019

2.3. Attributes should behave like data properties
... In particular, obj.attribute === obj.attribute must always hold, and so returning a new value from the getter each time is not allowed.

input.valueAsDate violate this.

Possible solutions:

  1. Relax the rule, allow returning mutable objects break obj.attribute === obj.attribute, or
  2. Suggest using method (like input.valueAsDate()) in future design, or
  3. Suggest avoiding return mutable objects, mark input.valueAsDate as mistake (should return a subclassed Date instance which throw when call setXXX).
@dbaron
Copy link
Member

dbaron commented Sep 30, 2019

@domenic
Copy link
Member

domenic commented Oct 1, 2019

There's nothing to be done here. valueAsDate violates this principle, and was poorly designed. If we could change it, we would, but it's too late now.

We should not change the design principles to accomodate bad design. Instead we should use them to prevent future instances of bad design.

I think (2) is already implemented in the design principles.

@hax
Copy link
Author

hax commented Oct 1, 2019

Instead we should use them to prevent future instances of bad design.

So could we make the text clear that valueAsDate is a mistake? Just like the doc mentions offsetTop

(A notable failure of the platform in this regard is getters like offsetTop performing layout; do not repeat this mistake.)

It will also helpful to add link to https://w3ctag.github.io/design-principles/#times-and-dates part.

but it's too late now.

I wonder if we can add a new api using Temporal to replace valueAsDate in the future? (relate to #101 )

@plinss plinss added this to the 2020-02-03-week milestone Jan 29, 2020
@hober
Copy link
Contributor

hober commented Jan 30, 2020

Hi,

I'm half-remembering a comment @dbaron made on one of our design reviews, in which he described two levels of "badness" WRT violating this invariant: in the "less bad" cases, a developer probably wouldn't be surprised at the invariant violation. Datetimes are possibly a good example of this. In the "more bad" cases, there isn't any indication by the nature of the API that it would violate this invariant.

David, do you remember where you made a comment along those lines? Do you think your reasoning applies in this case?

@dbaron
Copy link
Member

dbaron commented Jan 30, 2020

I think I remember making a comment like that more than once, but only with respect to other principles in our design principles document; I don't remember one regarding this principle in particular. The ones I remember were:

@dbaron
Copy link
Member

dbaron commented Jan 30, 2020

(And I don't particularly think it applies here; I think in this case the principle suggests that this should have been a method rather than an attribute.)

@torgo
Copy link
Member

torgo commented Feb 4, 2020

Maybe we can wrap this issue up this week and make a PR to the doc? We will discuss on the plenary call.

@dbaron
Copy link
Member

dbaron commented Mar 30, 2020

I think the section should probably say "if your attribute doesn't do this, you should consider making it a method instead", and then with that we should be able to close this issue as far as the design principles doc is concerned.

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 a pull request may close this issue.

6 participants