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

Uncomment types for Record, fix for .equals #876

Closed
wants to merge 2 commits into from

Conversation

nmn
Copy link
Contributor

@nmn nmn commented May 15, 2016

The Record type refs work as of Flow version 0.24.2.

The .equals method on _Iterable was causing Set.equals to not accept another Set. This seems to fix it.

The Record type refs work as of Flow version 0.24.2.

The .equals method on `_Iterable` was causing Set.equals to not accept another Set. This seems to fix it.
@ghost ghost added the CLA Signed label May 15, 2016
declare class Record<T: Object> {
static <T: Object>(spec: T, name?: string): /*T & Record<T>*/any;
static <T: Object>(spec: T, name?: string): T & Record<T>;
Copy link

Choose a reason for hiding this comment

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

Hi, are you sure that this line is correct? Isn't it supposed to be (I have checked it only with flow 0.26, so maybe it is working with 0.24):

static <T: Object>(spec: T, name?: string): Class<T & Record<T>>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right about this. I checked the other cases, and so I just uncommented the things I saw. I'll fix this.

@nmn
Copy link
Contributor Author

nmn commented Jul 7, 2016

Fixed outstanding issues. Added the getIn and setIn methods for Records.
If you have any suggestions for improving the type definitions let me know.

@tallixun would love another review

@ghost ghost added the CLA Signed label Jul 12, 2016
@philipp-spiess
Copy link
Contributor

philipp-spiess commented Aug 22, 2016

Hey @nmn. I would love to see your PR merged. However, there are a few method definitions missing. So far I found:

  • Record#updateIn()
  • Record#toJS()
  • Record#toObject()
  • Record#update()
  • Record#withMutations()
  • Record#delete()
  • Record#merge()

I think it would make sense to extend from KeyedCollection, since this is also the way it is implemented: https://github.com/facebook/immutable-js/blob/master/src/Record.js#L18

@ghost ghost added the CLA Signed label Aug 22, 2016
@lacker lacker added the flow label Dec 6, 2016
@lacker
Copy link

lacker commented Dec 6, 2016

So #878 refactored the flow types and also added tests for them. Would you mind updating this PR to merge those changes in, and also add a test that catches this? Thanks!

@kozlitinaelja
Copy link
Contributor

Hi there. I am starting handling backlog. I know it is been a while since you've submitted PR, sorry for such delay. The are some conflicts. Could you, please, rebase? 😅

leebyron added a commit that referenced this pull request Mar 3, 2017
The equals() function should really accept anything.

As discussed in #876
leebyron added a commit that referenced this pull request Mar 3, 2017
The equals() function should really accept anything.

As discussed in #876
leebyron added a commit that referenced this pull request Mar 7, 2017
leebyron added a commit that referenced this pull request Mar 7, 2017
leebyron added a commit that referenced this pull request Mar 8, 2017
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