Skip to content

Accessors should be allowed to be optional #54240

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

Open
5 tasks done
justinfagnani opened this issue May 13, 2023 · 7 comments
Open
5 tasks done

Accessors should be allowed to be optional #54240

justinfagnani opened this issue May 13, 2023 · 7 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@justinfagnani
Copy link

justinfagnani commented May 13, 2023

Suggestion

Right now accessors are barred from being declared optional. This is a DX regression for decorator users.

πŸ” Search Terms

accessor optional

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Allow accessors to be declared options;

πŸ“ƒ Motivating Example

Decorating a class field should require as few changes over a non-decorated field as necessary. Disallowing accessors from being optional adds an additional change.

Take a plain class:

class A {
  foo?: number;
}

Where you then want to make foo reactive with some library that vends decorators. This would be the change I would expect to make:

import {reactive, Base} from 'some-reactivity-lib';

class A extends Base {
  @reactive
  accessor foo?: number;
}

But 5.0 requires:

import {reactive, Base} from 'some-reactivity-lib';

class A extends Base {
  @reactive
  accessor foo: number | undefined;
}

I don't see a semantic problem with optional accessors. They behave very similar to optional fields (with standard field semantics). With useDefineForClassFields and this example:

class C {
  accessor foo?: number;
  
  bar?: number;
}

both foo and bar would exist on the runtime object even though they're optional, ie 'foo' in c === 'bar' in c.

πŸ’» Use Cases

More ergonomic decorator usage.

@jcalz
Copy link
Contributor

jcalz commented May 13, 2023

Are you talking about accessors in general? If so then this duplicates #14417.

Are you only talking about auto accessors? If so then you might want to edit to use that term specifically. Of course it's still possible they'd consider this subsumed by #14417.

@fatcerberus
Copy link

See also #16344, a PR which sought to implement this but seems to have fallen through (it’s now closed).

@RyanCavanaugh
Copy link
Member

@rbuckton Any thoughts?

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels May 23, 2023
@justinfagnani
Copy link
Author

We just finished porting Lit's decorators to the new spec, and started porting some consumers via a code mod. The lack of optional auto accessors is standing out as a DX wart. Any chance this could be considered @rbuckton?

@rbuckton
Copy link
Member

rbuckton commented Sep 6, 2023

accessor indicates a runtime transformation of a field to a get/set pair, and we don't currently allow get or set to be optional. Optionality is intended to indicate cases where the field itself may not exist, i.e., "x" in obj fails, which is made more apparent when you compile with --exactOptionalPropertyTypes. If you want accuracy (and portability with --exactOptionalPropertyTypes enabled), what you really want is exactly what is shown in the 5.0 example above, where you define the property as accessor x: Foo | undefined.

If you are already porting via a code mod, is there a reason that code mod cannot also drop the optionality and add | undefined?

Field optionality existed long before the native fields proposal, when it was completely feasible to declare an optional field and never actually define it (thus "x" in obj would return false). While --useDefineForClassFields means the field will always exist, it doesn't really match what ? was intended to mean. If we were being more strict, we might have disallowed ? on class fields. Then again, we allow ? on methods that are guaranteed to exist, and have for some time, despite that being primarily a feature for interface. I'm not strongly opposed to making allowing this, but it seems like a step in the wrong direction.

@justinfagnani
Copy link
Author

If you are already porting via a code mod, is there a reason that code mod cannot also drop the optionality and add | undefined?

The code mod can, but not everyone's going to use the code mode, and new code won't. We have 10s to 100s of thousands of users.

For newly written code, the users still have to write:

@property() accessor foo: string | undefined;

when previously they were writing:

@property() foo?: string;

Every additional required keyword or boilerplate adds up

it doesn't really match what ? was intended to mean

I'm not sure how other developers think of ?, but I have always intuitively thought of it as part of the type signature only, like shorthand for | undefined. If that's how it behaves now for class fields and methods, then it seems entirely consistent to me to make it work that way for auto-accessors, and instead of a step in the wrong direction, it's a step towards consistency.

@justinfagnani
Copy link
Author

justinfagnani commented Oct 28, 2024

As I port more decorator usage to standard decorators, the extra cost and keyword clutter of accessor foo: string | undefined vs accessor foo?: string is still very annoying.

When most people conceptually just want to write:

foo?: string;

but they need to use a decorator for reactivity, so they must do:

@reactive() foo?: string;

But decorators only work with auto-accessors, so they must do:

@reactive() accessor foo?: string;

But accessors aren't allowed to be optional, so they finally get to:

@reactive() accessor foo: string | undefined;

That's just a lot of extra boilerplate. The kind that honestly leads some people to JS forks.

Anything to reduce the sheer number of characters here would help, and optional accessors is basically the one thing within TypeScript's control.

aarongable pushed a commit to chromium/chromium that referenced this issue Mar 11, 2025
CL created by running the following script:
python3 ui/webui/resources/tools/codemods/jscodeshift.py \
  --transform ui/webui/resources/tools/codemods/389737066_migration_lit.js \
  --files `find ui/webui/resources/cr_components/theme_color_picker/ -name '*.ts'`

Also updated 389737066_migration_lit.js codemod to handle the case of
optional class members, to work around [1].

[1] microsoft/TypeScript#54240

Bug: 389737066
Change-Id: Ia7a886a94af2ee0bd32dda7f4c6edd458dd671b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6341751
Reviewed-by: Rebekah Potter <[email protected]>
Commit-Queue: Demetrios Papadopoulos <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1431107}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants