[PR] Implement versioned .js.flow #8135
Conversation
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
|
This pull request was exported from Phabricator. Differential Revision: D16431326 |
|
Rebased copy of #7842. See that PR for more info. |
| SMap.find_first_opt | ||
| (fun k -> | ||
| try Semver.satisfies ~include_prereleases:true k Flow_version.version | ||
| with Semver.Parse_error _ -> false) | ||
| types_versions |
There was a problem hiding this comment.
What this means is that if there are multiple available typesVersions entries that are satisfied by the current version, we will pick whichever is lowest alphabetically. I think this is a policy which will confuse users. I think we should instead pick the first entry that is satisfied by the current version.
This means that we should represent the types_versions as a list of pairs, in the order that they are written, rather than a Map, but since (a) it's likely to be a small list, and (b) we do an O(n) traversal here anyway, it shouldn't affect performance.
| @@ -93,6 +93,69 @@ let module_name_candidates ~options = | |||
| in | |||
| List.rev (name :: List.fold_left map_name [] mappers)) | |||
|
|
|||
| let types_versions_package_candidates reader = | |||
There was a problem hiding this comment.
Looks like this function returns a pair of (version, path) but only the path is used by the caller. Could you change it to return only the path?
| in | ||
| version_path) | ||
|
|
||
| let types_versions_candidates ~reader dir rel_path dirname = |
There was a problem hiding this comment.
This function and the associated code in node_paths_part.ml really make me nervous. I get worried whenever I see code that manipulates paths directly. There are a lot of opportunities to make mistakes, both for the original author and future maintainers.
I think it would help if I understood the motivation for this code better. What is happening here that couldn't be satisfied by reusing existing code? Fundamentally, it seems to me like we are looking in the package.json already for main to figure out what to resolve the module to, why can't we augment that logic to look at the typesVersions fields as well?
I have some more specific comments about this code, but I'd like to understand the context better before I dig deeper.
There was a problem hiding this comment.
Is it possible to write unit tests for this? I didn't found any runner in repository, but it has some OUnit tests
There was a problem hiding this comment.
We do have some unit tests, but they are only set up to run with Buck so we haven't open sourced them. @mroch, do you know what would be involved in setting up our unit tests to run with make?
|
Closing, since we'll actually merge the original copy |
Summary:
Proof of concept for #4917
Has bad performance on all resolutionsPerformance checks
Current measurements: https://gist.github.com/goodmind/65695c3b7a3aa0766812b15779a984dc
flow check --profileon codebasePull Request resolved: Implement versioned .js.flow #7842
Differential Revision: D16431326