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

Class properties #59

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Class properties #59

wants to merge 2 commits into from

Conversation

fizker
Copy link
Contributor

@fizker fizker commented Aug 8, 2019

Moved the section out of TS-only, since both ECMAScript and flow supports it.

@niieani
Copy link
Owner

niieani commented Aug 8, 2019

This compares type-checked private properties with runtime private properties - they're different, so this PR isn't accurate to that.

We can make a separate section about runtime private properties.

@fizker
Copy link
Contributor Author

fizker commented Aug 8, 2019

That's not true. The current version incorrectly claims that Flow does not support the concept of both public and private properties; it most certainly does, and it enforces this on a static type-checking level, same as TypeScript.

The actual runtime code is dependent on the compiler used and have nothing to do with Flow nor with static type-checking. There are probably other things that work fine at runtime even though it fails at type-checking time, such as read-only objects and exact objects. They are not mentioned anywhere, so I don't see the point of mentioning this either.


I can rewrite it to split the public and private properties apart. This makes sense since the two systems share the syntax for public properties, with the exception of some syntactic sugar that TypeScript supports.

The part for the private properties could get a note describing that TypeScript does not have actual private properties, if that seems appropriate. But then we move into runtime parts, which seems out of scope of this article.

- Put the public class properties under the shared-syntax header
- Rewrote the private part to be more to-the-point
Copy link
Owner

@niieani niieani left a comment

Choose a reason for hiding this comment

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

There's a distinction between TC39 private fields proposal, and private field type annotation in TypeScript. The PR mixes these two.

TypeScript supports:

  • TC39 private fields (fields with #, not transpiled in runtime)
  • private field type annotations (type-annotations only, not affecting emit)

While flow supports:

  • TC39 private fields (fields with #, not transpiled in runtime)
  • munged underscored fields (fields starting with _ are treated as private, type-annotations only, not affecting emit)

We should compare:

  • the first of both (it's identical, so 'same syntax')
  • the second of both

@@ -647,10 +647,71 @@ type C = Omit<A, B>;

However, Flow implementation is stricter in this case, as B have a property that A does not have, it would rise an error. In Typescript, however, they would be ignored.

## Private properties in classes

### flow
Copy link
Owner

Choose a reason for hiding this comment

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

The example below isn't flow specific. It works the same way as in TypeScript.

Copy link
Owner

Choose a reason for hiding this comment

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

If this could be specified as "same syntax" (without separation), then I'll accept.

}
```

### TypeScript
Copy link
Owner

Choose a reason for hiding this comment

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

This example can be removed, or moved to the part below (above Object callable property), since it duplicates the part that starts with:

Also, TypeScript have some syntactic sugar for setting the props through the constructor

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 this pull request may close these issues.

2 participants