Analysis refactor for #8425#8478
Conversation
DocumentSymbol.children is optional, so only set it when a symbol has nested entries. This avoids serializing leaf symbols with an empty children array. Signed-off-by: Pedro Castro <aspeddro@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9843819886
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ("suffix", `String package.suffix); | ||
| ("root_path", `String package.root_path); | ||
| ("project_files", string_set_to_yojson package.project_files); | ||
| ("dependencies_files", string_set_to_yojson package.dependencies_files); |
There was a problem hiding this comment.
When state_to_yojson serializes each package, it never emits the new package.dependencies field populated in Packages.new_bs_package; it jumps from project_files to dependencies_files. Server-side consumers calling this new JSON API can inspect dependency file paths but cannot see the dependency names/metadata this change adds, so packages with entries like "dependencies": ["@rescript/react"] are indistinguishable from packages with no dependency list. Please serialize package.dependencies here as its own field.
Useful? React with 👍 / 👎.
| |> List.map (fun path -> path @ ["place holder"]) | ||
| in | ||
| let dependencies = | ||
| match config |> Yojson_helpers.get "dependencies" with |
There was a problem hiding this comment.
For projects that still use the supported bs-dependencies alias, this new metadata is always empty because it only reads the dependencies key. The rest of analysis still treats bs-dependencies as an alias when resolving dependency files (analysis/src/find_files.ml:271-280), so server features using package.dependencies will report no dependencies even though the package is resolved and compiled with them. Please reuse the same alias-aware parsing for this field.
Useful? React with 👍 / 👎.
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8478 +/- ##
==========================================
- Coverage 74.42% 74.37% -0.06%
==========================================
Files 451 451
Lines 61459 61509 +50
==========================================
+ Hits 45743 45749 +6
- Misses 15716 15760 +44
🚀 New features to boost your workflow:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ad1af7b19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with | ||
| | None, None -> [] | ||
| | Some deps, None | _, Some deps -> | ||
| deps |> List.filter_map Yojson_helpers.string_opt |
There was a problem hiding this comment.
When rescript.json uses the supported qualified dependency form like { "name": "pkg", "features": [...] }, this filter keeps only JSON strings and silently omits that dependency from the analysis package state. The build config accepts both shorthand and qualified dependency entries (rewatch/src/config.rs defines Dependency as either Shorthand or Qualified and wires it to dependencies), so server-side consumers of this new metadata will see an incomplete dependency list for feature-gated packages. Please extract the name field from object entries instead of dropping them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is currently supported, @fhammerschmidt?
1 participant
This PR has five commits.
Cherry-pick of #8425