Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Warn against getter throwing exception since it is a side effect #408

Conversation

wolenetz
Copy link
Contributor

@wolenetz wolenetz commented Dec 20, 2022

Also includes some bikeshed indentation error fixes.


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

To be clear, in general I agree that you probably do not want your getters to throw, but I think in practice all platform object getters can throw. For instance, if their this doesn't have the correct platform object brand.

cc @domenic

index.bs Outdated
@@ -1359,6 +1359,13 @@ whether to use an attribute or a method.
- Attribute getters must not have any (observable) side effects.
If you have expected side effects, use a method.

- Attribute getters should not throw exceptions.
Exceptions are side effects.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's widely accepted that exceptions are side effects. E.g., I'm pretty sure typical side-effect-free functions are still allowed to do input validation.

I can kinda see how they can cause side effects in JavaScript, but only if you have listeners in place that end up modifying state in response to the exception.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is beside the point, because exceptions being side effects is not the reason getters should not throw. Getters should behave like regular JS properties, and these do not throw.
Also, typically, invalid state throws when set, it's not a good design to allow writes to go through only to realize you have invalid state when someone tries to read it.

Copy link
Member

@annevk annevk Feb 7, 2023

Choose a reason for hiding this comment

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

But then explain why getters should not throw and don't state that exceptions are side effects. (Which I see you are doing below, so I'm not sure how my comment was off point.)

As to invalid state, properties might not apply to all states the object can be in. This is quite common with HTMLInputElement for instance. I'm not talking about the value of the property here.

index.bs Outdated
@@ -1359,6 +1359,13 @@ whether to use an attribute or a method.
- Attribute getters must not have any (observable) side effects.
If you have expected side effects, use a method.

- Attribute getters should not throw exceptions.
Exceptions are side effects.
Updating existing APIs to include getters that throw exception must be avoided.
Copy link
Member

Choose a reason for hiding this comment

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

I think it can still be reasonable if that only happens in a new state that was not accessible before the update, but this is an interesting observation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given @annevk's comment, perhaps must should be watered down a bit?

@annevk
Copy link
Member

annevk commented Dec 21, 2022

(Sorry for not calling out these caveats earlier. I guess they didn't really occur to me when you filed the issue.)

index.bs Outdated
@@ -1359,6 +1359,13 @@ whether to use an attribute or a method.
- Attribute getters must not have any (observable) side effects.
If you have expected side effects, use a method.

- Attribute getters should not throw exceptions.
Exceptions are side effects.
Updating existing APIs to include getters that throw exception must be avoided.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Updating existing APIs to include getters that throw exception must be avoided.
Updating existing APIs to include getters that throw an exception must be avoided.

atanassov and others added 3 commits February 6, 2023 17:59
Co-authored-by: Sangwhan "fish" Moon <sangwhan@iki.fi>
Co-authored-by: Lea Verou <lea@verou.me>
Co-authored-by: Lea Verou <lea@verou.me>
index.bs Outdated
@@ -1359,6 +1359,13 @@ whether to use an attribute or a method.
- Attribute getters must not have any (observable) side effects.
If you have expected side effects, use a method.

- Attribute getters should not throw exceptions.
Exceptions are side effects.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Exceptions are side effects.
Getters should [behave like regular data properties](#attributes-like-data), and regular data properties do not throw exceptions when read. Furthermore, invalid state should generally be avoided by rejecting *writes*, not when data is *read*.

cynthia and others added 2 commits February 7, 2023 11:10
Co-authored-by: Lea Verou <lea@verou.me>
@@ -1359,6 +1359,10 @@ whether to use an attribute or a method.
- Attribute getters must not have any (observable) side effects.
If you have expected side effects, use a method.

- Attribute getters should not throw exceptions.
Getters should [behave like regular data properties](#attributes-like-data), and regular data properties do not throw exceptions when read. Furthermore, invalid state should generally be avoided by rejecting *writes*, not when data is *read*.
Copy link
Member

@LeaVerou LeaVerou Feb 7, 2023

Choose a reason for hiding this comment

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

Suggested change
Getters should [behave like regular data properties](#attributes-like-data), and regular data properties do not throw exceptions when read. Furthermore, invalid state should generally be avoided by rejecting *writes*, not when data is *read*.
Getters and setters should [behave like regular data properties](#attributes-like-data),
and regular data properties do not throw exceptions when read or set.
Invalid state should generally be avoided by rejecting *writes*, not when data is *read*,
and if setting data could throw exceptions, a method is generally preferable over a setter.

@LeaVerou LeaVerou mentioned this pull request Feb 7, 2023
@torgo torgo added this to the 2023-04-18-tokyo milestone Apr 6, 2023
cynthia and others added 2 commits April 20, 2023 11:21
Co-authored-by: Theresa O'Connor <hober0@gmail.com>
Copy link
Member

@torgo torgo left a comment

Choose a reason for hiding this comment

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

We discussed in today's call and we agreed to merge without Lea's proposed change for now.

@torgo torgo merged commit 0a41e3d into w3ctag:main Jun 6, 2023
@wolenetz wolenetz deleted the warn-against-getter-throwing-exception-since-it-is-a-side-effect branch July 27, 2023 00:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants