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

Implement versioned .js.flow #7842

Closed
wants to merge 4 commits into from

Conversation

goodmind
Copy link
Contributor

@goodmind goodmind commented Jun 21, 2019

Proof of concept for #4917
Has bad performance on all resolutions

Performance checks

Current measurements: https://gist.github.com/goodmind/65695c3b7a3aa0766812b15779a984dc

  1. Build flow binary
  2. Run flow check --profile on codebase
  3. Record results
  4. Repeat with regular flow binary and this PR, properly stopping all servers before running new binary

@goodmind goodmind added declarations Issues with library definitions or .js.flow features Experiment and removed CLA Signed declarations Issues with library definitions or .js.flow features labels Jun 21, 2019
@nmote nmote self-assigned this Jun 21, 2019
Copy link
Contributor

@nmote nmote left a comment

Choose a reason for hiding this comment

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

I skimmed this and overall it looks reasonable. I'm taking tomorrow off, but I'll import this and test it out internally on Monday. If performance looks good I'll give it a more thorough review. In the meantime, could you make the following changes?

First, could you update the summary to include a bit more information, including the results and methodology for the performance tests we've discussed on Discord?

Could you add a test that the Flow server restarts correctly if it sees a change in the flow field of a package.json file? See the package_incompatible function in module_js.ml. Based on the implementation it should restart, but let's just cover our bases.

Could you also add a test for the case where the flow property exists but contains no typesVersion property, as well as a test for the case where the typesVersion property is empty?

Also see my inline comments.

src/parser_utils/package_json.ml Outdated Show resolved Hide resolved
src/parser_utils/package_json.ml Outdated Show resolved Hide resolved
@goodmind
Copy link
Contributor Author

@nmote how can I test server restart?

@nmote
Copy link
Contributor

nmote commented Jul 22, 2019

I took a quick look and didn't see any existing examples of testing for server restart. With that in mind, I would be happy if you manually test that the server restarts and update the PR description with the steps you took.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@nmote
Copy link
Contributor

nmote commented Jul 26, 2019

It appears that we don't have Core's String module available for our internal build, so the build here is failing. I would have mentioned this sooner but this was masked by another build issue that was easy to fix.

@mroch, do you know if it would be reasonable to pull in Core's String? @goodmind, if not, could you rewrite this without using that library?

Sorry, I know it's not a good developer experience to have a separate build system that you can't see or use. Unfortunately there's not a whole lot we can do about that currently.

@goodmind
Copy link
Contributor Author

Yeah, it just needs substr_index, couldn't find this in jane street core

@goodmind
Copy link
Contributor Author

@nmote I removed Base

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@nmote
Copy link
Contributor

nmote commented Aug 13, 2019

Unfortunately the test you've added in this commit fails on Linux. Do you have access to a Linux development environment to look into this failure?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@goodmind
Copy link
Contributor Author

@nmote any news?

@nmote
Copy link
Contributor

nmote commented Sep 19, 2019

Yes, I've been trying to get to this. Sorry again for the delay 😞

I'm planning to rebase it past the ocamlformat change for you, since I'm not sure whether we have ocamlformat properly set up on GitHub yet. I was hoping to get to that yesterday, but ran out of time. I should be able to do that and give you a review today.

Summary:
<!--
  If this is a change to library defintions, please include links to relevant documentation.
  If this is a documentation change, please prefix the title with [DOCS].

  If this is neither, ensure you opened a discussion issue and link it in the PR description.
-->

Proof of concept for facebook#4917
<s>Has bad performance on all resolutions</s>

## Performance checks

Current measurements: https://gist.github.com/goodmind/65695c3b7a3aa0766812b15779a984dc

1. Build flow binary
2. Run `flow check --profile` on codebase
3. Record results
4. Repeat with regular flow binary and this PR, properly stopping all servers before running new binary
Pull Request resolved: facebook#7842

Differential Revision: D16431326

fbshipit-source-id: a18211b0160cf44f76f80b74ac246fd06d24c9d5
@nmote
Copy link
Contributor

nmote commented Oct 16, 2019

I've rebased this past the ocamlformat update and I put up #8135 with the updated code. Feel free to take what's there and push to your branch here.

I'll do a round of review over there, so that I don't have to wait for you to update this PR.

@nmote
Copy link
Contributor

nmote commented Oct 18, 2019

I've done a round of reviewing over there. Let me know when you get a chance to respond.

@goodmind goodmind force-pushed the types-versions-js.flow branch from f7bd065 to a0dba5d Compare October 19, 2019 15:03
@nmote
Copy link
Contributor

nmote commented Oct 24, 2019

I'm still concerned about the code in the types_versions_candidates function and node_paths_part.ml -- see my comment in #8135

@nmote
Copy link
Contributor

nmote commented Feb 3, 2020

Closing since there hasn't been any movement on this for some time

@nmote nmote closed this Feb 3, 2020
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.

3 participants