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] react/prefer-read-only-props fails if using Flow's $ReadOnly<T> #2770

Closed
wants to merge 3 commits into from

Conversation

@karolina-benitez
Copy link
Contributor

@karolina-benitez karolina-benitez commented Aug 26, 2020

No description provided.

@karolina-benitez karolina-benitez deleted the karolina-benitez:issue2472 branch Aug 26, 2020
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 26, 2020

@karolina-benitez why close/delete?

@ljharb ljharb added the flow label Aug 26, 2020
@karolina-benitez
Copy link
Contributor Author

@karolina-benitez karolina-benitez commented Aug 26, 2020

I was second guessing my understanding of the issue. Just to clarify, I need to adjust the rule to allow the object level annotation $ReadOnly to replace the need to add the individual annotation + to each attribute?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 26, 2020

To be honest I'm not super clear, I'm not familiar with Flow :-/ my personal approach here would be to write the tests first, and then blunder through the implementation until i'd figured out how to fix it.

@karolina-benitez
Copy link
Contributor Author

@karolina-benitez karolina-benitez commented Aug 26, 2020

Gotcha, from the Flow documentation:

props: $ReadOnly<{|
    dataTest?: string,
    className?: string,
  |}>,

is valid syntax but the current rule is returning an error because it's expecting each individual attribute to be annotated. Like so:

props: $ReadOnly<{|
    +dataTest?: string,
    +className?: string,
  |}>,

I think my initial understanding was correct and the first example should pass. I'll be adjusting my tests and will reopen the PR shortly

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 26, 2020

That seems right to me!

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 30, 2020

@karolina-benitez would you mind restoring the branch and reopening this PR, so i can keep it in sync with #2772?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Sep 1, 2020

To be specific, the desired workflow was to click the "restore branch" button on this PR, and then "reopen pull request". Pushing to the branch directly makes this PR unrecoverable.

@karolina-benitez
Copy link
Contributor Author

@karolina-benitez karolina-benitez commented Sep 1, 2020

Oh no, my mistake. What should be my next steps?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Sep 1, 2020

If this PR in fact is unrecoverable (if there's no "restore branch" button) then I think at this point we'll just leave the other two PRs open; i'll keep them in sync, and this one will have to be left as-is.

@karolina-benitez
Copy link
Contributor Author

@karolina-benitez karolina-benitez commented Sep 1, 2020

Yes it is unrecoverable, I don’t see a “restore branch” button. I appreciate your patience while I stumble around, I’m still trying to figure this all out

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Sep 1, 2020

No worries, it happens.

Feel free to hit me up on any medium in the future if you want to pair on this sort of thing :-)

@karolina-benitez
Copy link
Contributor Author

@karolina-benitez karolina-benitez commented Sep 1, 2020

That would be incredibly helpful, thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.