Skip to content

Make Flow work with your editor #18664

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

Merged
merged 1 commit into from
Apr 18, 2020
Merged

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 18, 2020

We typecheck the reconciler against each one of our host configs. yarn flow dom checks it against the DOM renderer, yarn flow native checks it against the native renderer, and so on.

To do this, we generate separate flowconfig files.

Currently, there is no root-level host config, so running Flow directly via flow CLI doesn't work. You have to use the yarn flow command and pick a specific renderer.

A drawback of this design, though, is that our Flow setup doesn't work with other tooling. Namely, editor integrations.

I think the intent of this was maybe so you don't run Flow against a renderer than you intended, see it pass, and wrongly think you fixed all the errors. However, since they all run in CI, I don't think this is a big deal. In practice, I nearly always run Flow against the same renderer (DOM), and I'm guessing that's the most common workflow for others, too.

So what I've done in this commit is modify the yarn flow command to symlink [Edit: ended up just copying the file instead because Flow had trouble detecting changes to a symlinked config file] the generated .flowconfig file into the root directory. The editor integration will pick this up and show Flow information for whatever was the last renderer you checked.

Everything else about the setup is the same, and all the renderers will continue to be checked by CI.

Test plan

  • Confirmed Flow worked in my editor (vscode) after running yarn flow dom once
  • Ran yarn flow native and confirmed it switched by adding a mistake to the native host config
  • Ran yarn flow-ci to confirm it ran against each renderer, again by adding a mistake to see if it would pick up on it

We typecheck the reconciler against each one of our host configs.
`yarn flow dom` checks it against the DOM renderer, `yarn flow native`
checks it against the native renderer, and so on.

To do this, we generate separate flowconfig files.

Currently, there is no root-level host config, so running Flow
directly via `flow` CLI doesn't work. You have to use the `yarn flow`
command and pick a specific renderer.

A drawback of this design, though, is that our Flow setup doesn't work
with other tooling. Namely, editor integrations.

I think the intent of this was maybe so you don't run Flow against a
renderer than you intended, see it pass, and wrongly think you fixed
all the errors. However, since they all run in CI, I don't think this
is a big deal. In practice, I nearly always run Flow against the same
renderer (DOM), and I'm guessing that's the most common workflow for
others, too.

So what I've done in this commit is modify the `yarn flow` command to
copy the generated `.flowconfig` file into the root directory. The
editor integration will pick this up and show Flow information for
whatever was the last renderer you checked.

Everything else about the setup is the same, and all the renderers will
continue to be checked by CI.
@acdlite acdlite requested review from sebmarkbage and gaearon April 18, 2020 02:23
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 18, 2020
@acdlite acdlite marked this pull request as draft April 18, 2020 02:24
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 18, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 91967aa:

Sandbox Source
polished-architecture-zcesc Configuration

@sizebot
Copy link

sizebot commented Apr 18, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 91967aa

@sizebot
Copy link

sizebot commented Apr 18, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 91967aa

@acdlite acdlite force-pushed the flow-in-editor branch 3 times, most recently from be2efde to 101e604 Compare April 18, 2020 02:49
@acdlite acdlite marked this pull request as ready for review April 18, 2020 02:50
@acdlite acdlite force-pushed the flow-in-editor branch 2 times, most recently from 3caccd1 to 8277273 Compare April 18, 2020 03:32
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Any way we could make it error if the file doesn’t match any generated configs? So that it’s more obvious why it doesn’t persist when you edit it.

@acdlite
Copy link
Collaborator Author

acdlite commented Apr 18, 2020

I made it log an error if the file was edited more recently than the template

@acdlite acdlite merged commit 52a0d6b into facebook:master Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants