-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[PR] Implement versioned .js.flow #8135
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = | ||
Module_hashtables.memoize_with_types_versions_candidates_cache ~f:(fun package_filename -> | ||
let package = | ||
match Module_heaps.Reader_dispatcher.get_package ~reader package_filename with | ||
| Some (Ok package) -> package | ||
| _ -> Package_json.empty | ||
goodmind marked this conversation as resolved.
Show resolved
Hide resolved
|
||
in | ||
let types_versions = Package_json.flow_types_versions package in | ||
let version_path = | ||
SMap.find_first_opt | ||
(fun k -> | ||
try Semver.satisfies ~include_prereleases:true k Flow_version.version | ||
with Semver.Parse_error _ -> false) | ||
types_versions | ||
Comment on lines
+105
to
+109
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What this means is that if there are multiple available This means that we should represent the |
||
in | ||
version_path) | ||
|
||
let types_versions_candidates ~reader dir rel_path dirname = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function and the associated code in 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
Node_paths_part.( | ||
let parts = get_node_module_path_parts rel_path dirname in | ||
match parts with | ||
| Some parts -> | ||
let package_root = | ||
if parts.package_root_index == -1 then | ||
rel_path | ||
else | ||
String.sub rel_path 0 parts.package_root_index | ||
in | ||
let package_filename = Filename.concat package_root "package.json" in | ||
let package_filename = Files.normalize_path dir package_filename in | ||
let version_path = types_versions_package_candidates reader package_filename in | ||
let filename = | ||
if parts.package_root_index == -1 then | ||
"" | ||
else | ||
String.sub | ||
rel_path | ||
(parts.package_root_index + 1) | ||
(String.length rel_path - (parts.package_root_index + 1)) | ||
in | ||
let rel_path = | ||
match version_path with | ||
| Some (_, version_path) -> | ||
(* Chop trailing "/" *) | ||
let tail = | ||
if filename = "" then | ||
version_path | ||
else | ||
Filename.concat version_path filename | ||
in | ||
Files.normalize_path package_root tail | ||
| None -> rel_path | ||
in | ||
(* If file points outside of root, resolve current package root *) | ||
let rel_path = | ||
if String_utils.string_starts_with rel_path package_root then | ||
rel_path | ||
else | ||
Files.normalize_path package_root filename | ||
in | ||
Some rel_path | ||
| _ -> None) | ||
|
||
let add_package filename = function | ||
| Ok package -> Module_heaps.Package_heap_mutator.add_package_json filename package | ||
| Error _ -> Module_heaps.Package_heap_mutator.add_error filename | ||
|
@@ -320,6 +383,19 @@ module Node = struct | |
let file_options = Options.file_options options in | ||
lazy_seq | ||
[ | ||
lazy | ||
( if SSet.mem dir node_modules_containers then | ||
lazy_seq | ||
( Files.node_resolver_dirnames file_options | ||
|> Core_list.map ~f:(fun dirname -> | ||
let rel_path = spf "%s%s%s" dirname Filename.dir_sep r in | ||
let rel_path_opt = types_versions_candidates ~reader dir rel_path dirname in | ||
match rel_path_opt with | ||
| Some rel_path -> | ||
lazy (resolve_relative ~options ~reader loc ?resolution_acc dir rel_path) | ||
| _ -> lazy None) ) | ||
else | ||
None ); | ||
lazy | ||
( if SSet.mem dir node_modules_containers then | ||
lazy_seq | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
(** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*) | ||
|
||
type node_module_path_parts = { | ||
top_level_node_modules_index: int; | ||
top_level_package_name_index: int; | ||
package_root_index: int; | ||
file_name_index: int; | ||
} | ||
|
||
type states = { | ||
before_node_modules: int; | ||
node_modules: int; | ||
scope: int; | ||
package_content: int; | ||
} | ||
|
||
let states = { before_node_modules = 0; node_modules = 1; scope = 2; package_content = 3 } | ||
|
||
(* Example of expected pattern: /base/path/node_modules/[@scope/otherpackage/@otherscope/node_modules/]package/[subdirectory/]file.js *) | ||
(* Returns indices: ^ ^ ^ ^ *) | ||
let get_node_module_path_parts (full_path : string) (node_modules_path_part : string) : | ||
node_module_path_parts option = | ||
let top_level_node_modules_index = ref 0 in | ||
let top_level_package_name_index = ref 0 in | ||
let package_root_index = ref 0 in | ||
let file_name_index = ref 0 in | ||
let part_start = ref 0 in | ||
let part_end = ref 0 in | ||
let state = ref states.before_node_modules in | ||
while !part_end >= 0 do | ||
part_start := !part_end; | ||
part_end := | ||
String_utils.substring_index ~start_pos:(!part_start + 1) Filename.dir_sep full_path; | ||
|
||
if !state == states.before_node_modules then ( | ||
let start = !part_start in | ||
let index = String_utils.substring_index ~start_pos:start node_modules_path_part full_path in | ||
if index == start then ( | ||
top_level_node_modules_index := start; | ||
top_level_package_name_index := !part_end; | ||
state := states.node_modules | ||
) | ||
) else if !state == states.node_modules || !state == states.scope then | ||
let index = !part_start + 1 in | ||
let ch = full_path.[index] in | ||
let has_scope = ch == '@' in | ||
if !state == states.node_modules && has_scope then | ||
state := states.scope | ||
else ( | ||
package_root_index := !part_end; | ||
state := states.package_content | ||
) | ||
else if !state == states.package_content then ( | ||
let index = | ||
String_utils.substring_index ~start_pos:!part_start node_modules_path_part full_path | ||
in | ||
if index == !part_start then state := states.node_modules | ||
) else | ||
state := states.package_content | ||
done; | ||
|
||
file_name_index := !part_start; | ||
|
||
if !state > states.node_modules then | ||
Some | ||
{ | ||
top_level_node_modules_index = !top_level_node_modules_index; | ||
top_level_package_name_index = !top_level_package_name_index; | ||
package_root_index = !package_root_index; | ||
file_name_index = !file_name_index; | ||
} | ||
else | ||
None |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
[ignore] | ||
|
||
[include] | ||
|
||
[libs] | ||
|
||
[options] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Found 0 errors |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?