feat(core): implement `DependencyGraph` #4929

arendjr · 2025-01-20T16:22:21Z

Summary

Implements a major part of #3307. Through the DependencyGraph we can conveniently look up which file a given import statement or expression is pointing towards. Note this only works for JS/TS files (no CSS @import for now).

~~Based on an unmerged PR for oxc-resolver (oxc-project/oxc-resolver#360), so I'll wait with merging this until that one is merged.~~ Update: Bumped oxc-resolver to 4.0.

I major part of getting this to work with oxc-resolver was connecting our types for package.json and tsconfig.json with the new traits used by oxc-resolver. For full compatibility with package.json this even required storing and querying arbitrary JSON values, so I've created the biome_json_value crate for this, exporting types such as JsonValue, JsonArray, JsonObject and JsonString. These can be created relatively cheaply from our CST representations, while JsonString also contains fully unescaped string values.

Caveats

The DependencyGraph is not yet available to lint rules, so we require one more step before making use of it.
We still have no method of updating the DependencyGraph in the LSP, so information is likely to get stale when files are created/moved/deleted.

Test Plan

Tests added, but I'm still iterating on this a bit: It's a bit of a hassle that symlinks don't work in the memory FS...

This introduces `PackageJson` and `TsConfig` traits. The old structs have been renamed to `PackageJsonSerde` and TsConfigSerde` respectively. Both are behind the `fs_cache` feature flag now. `serde` as a dependency has become optional and is only needed for the `fs_cache` feature too. For now, I have opted not to replace the `FileSystem::read_to_string()` method with trait-specific versions yet, since this functionality is now all encapsulated within the `FsCache`. Consumers that wish to use custom implementation of the manifest traits most likely want to use a custom cache altogether (I know this will be true for Biome at least), so I didn't see much reason for additional complexity and breaking changes there now. ~~I'm still working on a Biome PoC based on this, so I'll keep the PR on Draft until I can verify everything works. Feedback is certainly already welcome!~~ **Update:** See biomejs/biome#4929 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

codspeed-hq · 2025-01-20T17:13:55Z

CodSpeed Performance Report

Merging #4929 will not alter performance

_{Comparing arendjr:dependency-graph (9a03e7e) with next (7dc32e6)}

Summary

✅ 95 untouched benchmarks

ematipico

I left questions to better understand the direction of the PR

justfile

crates/biome_json_value/src/json_value.rs

crates/biome_package/src/node_js_package/package_json.rs

crates/biome_rowan/src/syntax_factory/raw_syntax.rs

crates/biome_service/src/diagnostics.rs

crates/biome_service/src/workspace/server.rs

arendjr · 2025-01-21T19:36:53Z

I've addressed most of the feedback, but I'm leaving improvements for other languages out of scope for now. I feel it's more productive now to iterate on getting JS support right, than to introduce a whole bunch of additional traits to make CSS support possible.

Conaclos · 2025-01-21T21:06:07Z

crates/biome_package/src/node_js_package/package_json.rs

+                    if let Some(value) = value.into_json_value() {
+                        result.raw_json.insert(key.into(), value.into());


Why do we need raw JSON here?

It’s because oxc-resolver wants us to implement these getter functions that can pull fields out of the package.json using arbitrary paths.

I considered just limiting our support to a fixed set of paths, so we can pull it from pre-parsed static structures, but I tried a similar thing in the oxc-resolver repo when I introduced the traits, and it made the tests fail. I haven’t gone too deep into figuring out why, but I figured I just play it safe :)

crates/biome_deserialize/src/lib.rs

Conaclos · 2025-01-21T21:11:16Z

Oh finally a dependency graph 🍬 ❤️

For full compatibility with package.json this even required storing and querying arbitrary JSON values, so I've created the biome_json_value crate for this, exporting types such as JsonValue, JsonArray, JsonObject and JsonString. These can be created relatively cheaply from our CST representations, while JsonString also contains fully unescaped string values.

Could we directly create JsonValue, JsonArray, JsonObject and JsonString from DeserailizableValue (simply by implementing Deserializable on a sum type of these types)? This could avoid introducing the into_json_value() method on the DeseraiizableValue trait.

arendjr · 2025-01-21T21:41:00Z

Could we directly create JsonValue, JsonArray, JsonObject and JsonString from DeserailizableValue (simply by implementing Deserializable on a sum type of these types)? This could avoid introducing the into_json_value() method on the DeseraiizableValue trait.

That’s what I originally tried to use, but I couldn’t get it to work. I think it was actually based on a misunderstanding of how these traits are supposed to work. JsonValue already is a sum type, and AnyJsonValue already is too. That’s why the into_json_value() method works and can simply return Some(self). But JsonValue already implements Deserializable, so I tried to use it as you suggest, but couldn’t get that to work with the deserialize method of PackageJson. Maybe I missed something?

Conaclos

I tried to use it as you suggest, but couldn’t get that to work with the deserialize method of PackageJson. Maybe I missed something?

I made a code suggestion.
Let me know if it works and If you have any question. biome_deserialize is not an easy crate. I am quite satisfied of the provided abstractions. This why shy I am picky 😅

I also left more suggestions for the PR.

Conaclos · 2025-01-21T22:43:25Z

crates/biome_package/src/node_js_package/package_json.rs

+                key => {
+                    if let Some(value) = value.into_json_value() {
+                        result.raw_json.insert(key.into(), value.into());
+                    }


JsonValue already is a sum type
JsonValue already implements Deserializable, so I tried to use it as you suggest, but couldn’t get that to work with the deserialize method of PackageJson. Maybe I missed something?

So you already did the hard work 😅
I think you just need to write something like:

Suggested change

key => {

if let Some(value) = value.into_json_value() {

result.raw_json.insert(key.into(), value.into());

}

key_text => {

if let (Some(key), Some(value)) = (

JsonString::deserialize(ctx, &key, ""),

JsonValue::deserialize(ctx, &value, key_text),

) {

result.raw_json.insert(key, value);

}

}

or using type inference:

key_text => { if let (Some(key), Some(value)) = ( Deserializable::deserialize(ctx, &key, ""), Deserializable::deserialize(ctx, &value, key_text), ) { result.raw_json.insert(key, value); } }

I could also rename raw_json as rest (this will match the naming convention of the rest biome_deserialize_macros field attribute) or extra.

This should allow you to remove the DeserializableValue::into_json_value and also the dependency between biome_json_value crate and biome_json_syntax (assuming you also remove the From<AnyJsonValue> impl and related impls).

You were totally right, thanks! I guess I just chased my tail a bit before fixing it 🤣

crates/biome_json_value/src/json_value.rs

Conaclos · 2025-01-21T22:55:16Z

crates/biome_json_value/src/json_value.rs

+///
+/// See the [module-level documentation](self) for more info.
+#[derive(Clone, Debug, Default, Deserializable, PartialEq)]
+pub struct JsonObject(IndexMap<JsonString, JsonValue, FxBuildHasher>);


The use of IndexMap is certainly a good fit here in terms of data modeling.
However, looking at the usage in the impl of oxc_resolver::ImportsExportsMap, I think it could be better to just use Vec<(JsonString, JsonValue)> or even Box<[(JsonString, JsonValue)]>.
This avoids a dependency on the indexmap crate.
Eventually I think Biome should completely get rid of its dependency on indexmap.

Note that using Box<[(JsonString, JsonValue)]> will require some change in impl Deserializable for PackageJson because we will have to build a vec and after the loop to turn it into a boxed slice before assigning it to PackageJson::raw_json.

What's the motivation for getting rid of indexmap? I'm not strongly opinionated towards it, but I think we should at least have an alternative that provides equivalent performance for lookups, which a Vec wouldn't provide.

Is order preserving an important thing in our context? We could use FxHashMap instead.

What's the motivation for getting rid of indexmap?

We primarily used ordered set (indexmap::IndexSet) in our configuration system.
However, ordered set was not a good fit in the places it was used (It created erroneous configuration merging).
Removing this usage, this leads to the use of the indexmap only in some places.
And, most of the time, a Vec, a HashSet or HashMap is sufficient.

Now we only have a few places that use the indexmap crate.
This raises the question of Biome's dependency on this crate.
If we only use indexmap two or three times in the codebase, it might be worth removing it.

I think we do rely on the order preservation here, yes. This is because the resolving of imports and exports in package.json iterates through the keys to find matching entries.

crates/biome_json_value/src/json_value.rs

arendjr requested review from a team January 20, 2025 16:22

arendjr mentioned this pull request Jan 20, 2025

feat!: PackageJson and TsConfig traits oxc-project/oxc-resolver#360

Merged

arendjr changed the base branch from main to next January 20, 2025 16:24

Implement dependency graph

6368854

arendjr force-pushed the dependency-graph branch from caf644d to 6368854 Compare January 20, 2025 16:28

github-actions bot removed A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools A-LSP Area: language server protocol L-CSS Language: CSS A-Diagnostic Area: diagnostocis A-Changelog Area: changelog L-HTML Language: HTML L-Grit Language: GritQL labels Jan 20, 2025

arendjr mentioned this pull request Jan 20, 2025

📎 Make DependencyGraph available to lint rules #4930

Closed

ematipico reviewed Jan 21, 2025

View reviewed changes

Feedback + test fixes

e99dfad

github-actions bot added the A-Formatter Area: formatter label Jan 21, 2025

arendjr requested a review from ematipico January 21, 2025 19:37

Fixes

3f166ea

Conaclos reviewed Jan 21, 2025

View reviewed changes

crates/biome_deserialize/src/lib.rs Outdated Show resolved Hide resolved

Conaclos reviewed Jan 21, 2025

View reviewed changes

arendjr force-pushed the dependency-graph branch from 7dc0070 to 27adb31 Compare January 22, 2025 10:10

Fixes + feedback

9a03e7e

arendjr force-pushed the dependency-graph branch from 27adb31 to 9a03e7e Compare January 22, 2025 13:14

arendjr merged commit 481ad24 into biomejs:next Jan 22, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): implement `DependencyGraph` #4929

feat(core): implement `DependencyGraph` #4929

arendjr commented Jan 20, 2025 •

edited

Loading

codspeed-hq bot commented Jan 20, 2025 •

edited

Loading

ematipico left a comment

arendjr commented Jan 21, 2025

Conaclos Jan 21, 2025

arendjr Jan 21, 2025

Conaclos commented Jan 21, 2025

arendjr commented Jan 21, 2025

Conaclos left a comment •

edited

Loading

Conaclos Jan 21, 2025

arendjr Jan 22, 2025

Conaclos Jan 21, 2025 •

edited

Loading

arendjr Jan 22, 2025

Conaclos Jan 22, 2025

arendjr Jan 22, 2025

		if let Some(value) = value.into_json_value() {
		result.raw_json.insert(key.into(), value.into());

-                key => {
-                    if let Some(value) = value.into_json_value() {
-                        result.raw_json.insert(key.into(), value.into());
-                    }
+                key_text => {
+                    if let (Some(key), Some(value)) = (
+                        JsonString::deserialize(ctx, &key, ""),
+                        JsonValue::deserialize(ctx, &value, key_text),
+                    ) {
+                        result.raw_json.insert(key, value);
+                    }
+                }

feat(core): implement DependencyGraph #4929

feat(core): implement DependencyGraph #4929

Conversation

arendjr commented Jan 20, 2025 • edited Loading

Summary

Caveats

Test Plan

codspeed-hq bot commented Jan 20, 2025 • edited Loading

Merging #4929 will not alter performance

Summary

ematipico left a comment

Choose a reason for hiding this comment

arendjr commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conaclos commented Jan 21, 2025

arendjr commented Jan 21, 2025

Conaclos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conaclos Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(core): implement `DependencyGraph` #4929

feat(core): implement `DependencyGraph` #4929

arendjr commented Jan 20, 2025 •

edited

Loading

codspeed-hq bot commented Jan 20, 2025 •

edited

Loading

Conaclos left a comment •

edited

Loading

Conaclos Jan 21, 2025 •

edited

Loading