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

fix(Flow Types): Fixed Flow typing to allow for omitting options when using the options param #315

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FermiDirak
Copy link

@FermiDirak FermiDirak commented Oct 19, 2018

Currently options are an all or nothing parameter. This means if you want to specify the tabStop, you must specify every other option if using Flow. This PR splits the Options type to OptionParams and Options so that options can be independently specified when using the options param.

@ghost

This comment has been minimized.

@FermiDirak FermiDirak changed the title Fixed Flow typing to allow for omitting options when using the options param fix(Flow Types): Fixed Flow typing to allow for omitting options when using the options param Oct 19, 2018
…plit the Options type to OptionParams and Options so that properties can be independently specified in options
@FermiDirak
Copy link
Author

FermiDirak commented Oct 19, 2018

I'm open to adding tests for this PR

@vvo
Copy link
Contributor

vvo commented Oct 19, 2018

@FermiDirak why not just updating the already in place Options type definition (warning: I am new to Flow)

@FermiDirak
Copy link
Author

FermiDirak commented Oct 19, 2018

@vvo Flow isn't able to infer default values in a function signature, so when you consume reactElementToJsxString and use Flow, the Options argument must be specified with all its properties defined.

The problem with updating the Options type is that some of the methods in the library require all of optionss properties to be defined. The way to circumvent Flow not inferring defaults is to create a separate type for the function signature.

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.

3 participants