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

Updated PropertyDescriptor type to use the latest flow core definition #1960

Merged
merged 2 commits into from
Jun 4, 2019

Conversation

tbezman
Copy link
Contributor

@tbezman tbezman commented May 8, 2019

We're updating our MobX to version 4 at Coinbase. We came across an issue with the flow typings for PropertyDescriptor. The first commit is us just running prettier. The second commit is where we bring in the Flow core definition of PropertyDescriptor.

Can we make sure this is released under version 4?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.341% when pulling b9ae865 on tbezman:mobx4-master into 2b09e99 on mobxjs:mobx4-master.

@ItamarShDev
Copy link
Member

can you please remove the prettier commit?
prettifying better be done in a single PR to be as clean as possible

thank you

@capaj
Copy link
Member

capaj commented Jun 2, 2019

@ItamarShDev if we don't squash it it will be displayed as two separate commits in the git history-who cares if it's 2 PRs or 1? The source of truth is the git history, not the github PR history.
Also I think we have at least a few PRs we have merged that mixed changes to functionality and formatting so I don't think this is a rule we follow here in mobx org.

Copy link
Member

@capaj capaj left a comment

Choose a reason for hiding this comment

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

LGTM

@mweststrate
Copy link
Member

@tbezman ideally keep prettier out next time indeed (or do it as separate PR), makes reviewing and merging harder. I'll try to cherry-pick the descriptor commit to the master branch as well, hope it will work out :)

@mweststrate mweststrate merged commit 923d303 into mobxjs:mobx4-master Jun 4, 2019
@mweststrate
Copy link
Member

Released as 4.10.0 / 5.10.0

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.

5 participants