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

[typescript] Update typings for beta.4 and beta.5 #7793

Merged
merged 5 commits into from Aug 18, 2017

Conversation

sebald
Copy link
Member

@sebald sebald commented Aug 17, 2017

ℹ️ I will gather any updated here until Friday and then merge it, so we hopefully have the typings ready for the next beta.


This patch contains the following updates regarding the typings:


  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@sebald sebald self-assigned this Aug 17, 2017
Sebastian Sebald added 3 commits August 17, 2017 08:45
- Regenerate enzyme + react typings to fix "subsequent declaration" issue
- Regenerate `yarn.lock` so enzyme and react don't have a "subsequent declaration" issue...
@sebald
Copy link
Member Author

sebald commented Aug 17, 2017

@oliviertassinari The API change in #7740 is super awesome BTW! Loving it 🤗

Also, since we now have tests for the typings. People could just commit snippets that don't work and I could just add them to the existing tests. Would you approve a page in the docs that describes this process? (and maybe some info on how the typings work in general)

@oliviertassinari
Copy link
Member

@sebald Yeah, I think that we could document the process somewhere, we have a test/README.md file as well as a CONTRIBUTING.md. Maybe a new documentation page around typing (flow and typescript) will answer the problem?

@vyrotek
Copy link

vyrotek commented Aug 17, 2017

Is this the right place to provide feedback about the typings? If not please let me know what I should do.

@sebald It looks like Drawer's docked field only accepts true | false and not a boolean. I currently set docked from a state value in order to handle switching between fullscreen and mobile versions of my sidebar menu.

Example
<Drawer docked={this.state.fullscreen}>

I get this error:
Types of property 'docked' are incompatible. Type 'boolean' is not assignable to type 'false | undefined'

@sebald
Copy link
Member Author

sebald commented Aug 18, 2017

@vyrotek Yes, that's the right place. Thank you for reporting this. Will have a look at it.

@sebald
Copy link
Member Author

sebald commented Aug 18, 2017

Well, this this is a very weird behaviour. Thought that was fixed in microsoft/TypeScript#10577

Here is a minimal example to reproduce:

type One = {
  kind: true;
  foo: string;
} 

type Two = {
  kind: false;
  foo: boolean;
}

type Options = One | Two;

const fn = (options: Options) => { /* ... */ };

const optionsOne = { kind: true, foo: 'bar' };

/**
 * ERROR:
 * Types of property 'kind' are incompatible.
 * Type 'boolean' is not assignable to type 'false'.
 */
fn(optionsOne);

/**
 * No Error.
 */
fn({ kind: true, foo: 'bar' })

Link to the playground

For now we don't use them, maybe Reason -> microsoft/TypeScript#17882 helps to find a way to make use of them.
@sebald
Copy link
Member Author

sebald commented Aug 18, 2017

I have removed the discriminated unions for now from the typings as they did not work as I expected.

Some more info: The whole idea was to use discriminated unions to have a strongly typed API and make some props only available, if other props have certain values. For example, if <ListItem button={true} /> is set the component is rendered as a button, thus ButtonBaseProps can also be applied to the component. Now they're always allowed, but may not have any actual impact on runtime.

@sebald
Copy link
Member Author

sebald commented Aug 18, 2017

Going to merge this for now, so we finally have some typings in the next beta!
Please open a new issue + @sebald so I'll get notified! :)

@sebald sebald merged commit 5e98ee2 into v1-beta Aug 18, 2017
@sebald sebald deleted the typings-update-beta.5-6 branch August 18, 2017 08:39
@oliviertassinari
Copy link
Member

Looks good :)

@sebald
Copy link
Member Author

sebald commented Aug 18, 2017

Thanks :)

@marcusjwhelan
Copy link

@sebald How will we know when the typings have been released?

@sebald
Copy link
Member Author

sebald commented Aug 18, 2017

@marcusjwhelan guess they will be available with the next beta. Check its changelog :)

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

Successfully merging this pull request may close these issues.

None yet

5 participants