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 loading biome.config.js #1825

Closed
wants to merge 7 commits into from
Closed

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Feb 14, 2024

Summary

This PR implements loading config from biome.config.js. I don't think this necessarily needs to be merged as is. Instead, I hope it can help us to find answers to the following two questions:

  • Do we think Boa is a suitable engine to use within Biome? I haven't got performance metrics yet, so I'll try to look into that a bit more, but my first impression is positive.
    • Main gotcha I ran into is that I had an issue with their module loader, which is resolved in main. So the dependency is pinned to their repository rather than a release for now.
  • What do we think of this approach of loading JS config? It is fully optional, and resolves to a PartialConfiguration exactly like the config we load directly from a JSON file.
    • Main thing I still want to test myself is whether imports indeed work, so that I can emulate extends with pure JS syntax.

Note that this intentionally doesn't support TypeScript yet, so I didn't need a transpiler just now.

Test Plan

Added a test case for loading JS config.

Copy link

netlify bot commented Feb 14, 2024

Deploy Preview for biomejs canceled.

Name Link
🔨 Latest commit ae08477
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65cfb19f4a18ba0008cd67c5

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Project Area: project labels Feb 14, 2024
@@ -82,7 +82,7 @@ pub struct CliOptions {

impl CliOptions {
/// Computes the [ConfigurationBasePath] based on the options passed by the user
pub(crate) fn as_configuration_base_path(&self) -> ConfigurationBasePath {
pub(crate) fn to_configuration_base_path(&self) -> ConfigurationBasePath {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the distraction, just renamed this to follow Rust's naming conventions: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

fn format_with_js_configuration() {
let mut console = BufferConsole::default();
let mut fs = MemoryFileSystem::default();
fs.set_working_directory("/");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Boa's default module loader didn't like it if there's no working directory, so I had to add one to the memory filesystem. Note the main module is currently loaded from a string that is read from disk by our config finder, so I expect this might need a little bit more before loading external modules will work through the memory file system.

@ematipico
Copy link
Member

As far as I understood, Boa is an interpreter, not an engine/runtime. Right?

Can we do things like this?

// biome.js

let severity = "warn";
if (isProd()) {
	severity = "error"
}

export const config = {
	linter: {
		rules: {
			correctness: {
				noDebugger: severity
			}
		}
	}
}

///
/// The data structures that need to be deserialized have to implement the [Deserializable] trait.
/// For most data structures, this can be achieved using the
/// [biome_deserialize_macros::Deserializable] derive.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this comment is not entirely correct, since the function uses Serde traits for now, instead of our Deserializable. If we agree on picking Boa, I would optimize this function to use Deserializable again so we can go straight from boa_engine::JsValue to our own types, instead of using serde_json::Value as intermediary.

@arendjr
Copy link
Contributor Author

arendjr commented Feb 15, 2024

As far as I understood, Boa is an interpreter, not an engine/runtime. Right?

It is an interpreter, but that is also an engine/runtime. Internally it compiles the JavaScript to bytecode, which it operates on for execution. The only thing it lacks compared to "big" engines such as V8 and JavaScriptCore is a JIT engine. This basically creates the following trade-off for us:

  • Engines with a JIT compiler can offer much higher throughput than engines with only an interpreter can. So Boa would be a poor choice for us if we anticipate long-running scripts inside Biome.
  • JIT engines are also much more complex. Because of this they increase the size of our binary much more than Boa does, and they also tend to have slower startup times. If we want to keep latency low, Boa is an excellent choice.

Btw, there is also a boa_runtime crate, but it appears the only thing it does so far is provide a convenient console.log() implementation for use with Boa.

Can we do things like this?

// biome.js

let severity = "warn";
if (isProd()) {
	severity = "error"
}

export const config = {
	linter: {
		rules: {
			correctness: {
				noDebugger: severity
			}
		}
	}
}

Yes! That's one of the main benefits of a JS config.

@arendjr
Copy link
Contributor Author

arendjr commented Feb 15, 2024

Btw, since I mentioned latency with Boa is very good: I did some preliminary tests on my admittedly not very beefy laptop (it has a low-power Intel CPU with only 2 performance cores) and evaluating a simple script in Boa takes 11ms in debug builds. I would expect much better performance in a release build still (quite possibly bringing it to <1ms), but I have to set up some proper benchmarks for that.

@anonrig
Copy link
Contributor

anonrig commented Feb 16, 2024

As far as I understood, Boa is an interpreter, not an engine/runtime. Right?

In order to do those things, for example we need to also implement process.env.*. Without implementing a subset of Node.js functionality like node:fs or process.env, I don't see the value of adding .js files. Maybe, I'm missing something?

@arendjr
Copy link
Contributor Author

arendjr commented Feb 16, 2024

Without implementing a subset of Node.js functionality like node:fs or process.env, I don't see the value of adding .js files.

I would certainly consider implementing such features within scope of our runtime, even if it's not in the initial PoC.

@github-actions github-actions bot added the A-Tooling Area: internal tools label Feb 16, 2024
@arendjr
Copy link
Contributor Author

arendjr commented Feb 16, 2024

config evaluation took 165.24µs
exported config = {
 foo: "bar"
}
js_config/js_config_simple
                        time:   [178.13 µs 178.85 µs 179.84 µs]
                        thrpt:  [228.07 KiB/s 229.33 KiB/s 230.26 KiB/s]

Even better than I had hoped for. Less than 0.2ms for instantiating the engine, parsing and evaluating the script and converting the result to our PartialConfiguration type. Yes, this is a super tiny config, but I think this sets a good baseline.

@arendjr
Copy link
Contributor Author

arendjr commented Feb 16, 2024

@ematipico @Conaclos I wouldn't want to merge this if it's too controversial, so I'd like to ask your opinion:

  • Do you agree with the direction of continuing with Boa as our JS engine?
  • Any verdict on the addition of JS configs? I think it may be a good idea to rename the config file to biome.experimental-config.js at least, to clarify it's not a finished feature yet.

@ematipico
Copy link
Member

  • I would name the file biome.js ( no config in the file name, let's follow biome.json)
  • other than adding the experimental word in the configuration, I'd like to have a warning diagnostic
  • I would like to have some tests (snapshots) on invalid configurations, and make sure that the same constraints we have in the JSON file are also met in the JS file e.g. our max allowed value for lineWidth is 320
  • I would like to see more errors tests. While speed is a stake matter, also DX needs to be high quality. If we can't provide a text range of an invalid value, that's a red flag IMHO
  • I would like to understand if dynamic values can still be evaluated. As I said before, we have a max allowed value of 320. If a user sets 200 * 20, are we able to catch the error and emit a diagnostic?

These are important requirements

Copy link

codspeed-hq bot commented Feb 16, 2024

CodSpeed Performance Report

Merging #1825 will improve performances by 36.33%

Comparing arendjr:js-config (ae08477) with main (aec8905)

Summary

⚡ 1 improvements
✅ 92 untouched benchmarks

🆕 1 new benchmarks

Benchmarks breakdown

Benchmark main arendjr:js-config Change
🆕 js_config[js_config_simple] N/A 4.5 ms N/A
big5-added.json[cached] 3.1 ms 2.3 ms +36.33%

@SuperchupuDev
Copy link
Member

how does this js config system perform compared to the json one?

@Conaclos
Copy link
Member

Boa is a very promising/interesting option and I like the idea o using it. However, I am a bit frustrated by the fact we didn't test other options...

Regarding js config, I could delay its support. We are planning to change some parts of our config. It will be difficult or even impossible to provide a way to automatically migrate the ks config. I am not against this support, but I could wait until some users ask for it?

@arendjr
Copy link
Contributor Author

arendjr commented Feb 17, 2024

@ematipico I think your concerns are good ones, and I don't see any issue addressing them except one:

If we can't provide a text range of an invalid value, that's a red flag IMHO

I think we may be able to provide a text range that would point to the export statement where the config is produced, but I'm afraid we wouldn't be able to point to individual values. That would require an engine to track spans at runtime for all its values. I'm not aware of any engine that does this, since it would come with a significant runtime overhead.

@Conaclos said:

However, I am a bit frustrated by the fact we didn't test other options...

I took the JIT-powered engines out of the equation because you were worried about increasing the binary size and others expressed concerns about latency of spinning up the engine (both valid concerns!). But frankly, that only leaves the interpreters and there aren't that many good ones out there that we can use:

Boa covers the platforms we need, has good ECMAScript coverage, and is easy to integrate since it's also written in Rust. The latter I consider an extra advantage, because it means Biome contributors probably wouldn't have much issue jumping in to help with engine work too.

Of course, if you would like to reconsider JIT engines, I guess I could try a Deno PoC as I originally intended :) But I have played around with this before, and it's not going be as easy as working with Boa.

Regarding js config, I could delay its support. #689. It will be difficult or even impossible to provide a way to automatically migrate the ks config. I am not against this support, but I could wait until some users ask for it?

I think GritQL should be able to support migrations for most common JS configs :) But yeah, given the hesitance towards JS configs, let's postpone this for now so we can focus on the choice of engine.

@Conaclos
Copy link
Member

Conaclos commented Feb 17, 2024

Boa covers the platforms we need, has good ECMAScript coverage, and is easy to integrate since it's also written in Rust. The latter I consider an extra advantage, because it means Biome contributors probably wouldn't have much issue jumping in to help with engine work too.

I agree with all your points, and I think Boa is a great fit foor us. I was wondering if llrt or workerd could be worth to test.

I think GritQL should be able to support migrations for most common JS configs :) But yeah, given the hesitance towards JS configs, let's postpone this for now so we can focus on the choice of engine.

+1

@arendjr
Copy link
Contributor Author

arendjr commented Feb 17, 2024

I was wondering if llrt or workerd could be worth to test.

Both of those are very explicitly tailored to server-side use-cases though. For workerd the docs even explicitly mention "Designed for servers, not CLIs nor GUIs.". Neither of them are independent engines either, llrt is based on QuickJS (they also don't publish Windows binaries), while workerd is based on V8 (and comes with a full JIT overhead).

@arendjr
Copy link
Contributor Author

arendjr commented Feb 25, 2024

Closing this one since we won’t do JS configs for now. I’ll probably make a new effort with Boa when starting on actual JS plugins.

@arendjr arendjr closed this Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Core Area: core A-Project Area: project A-Tooling Area: internal tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants