From 15ae074619c6e5b1c7facebc35ceb245df249fe1 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Fri, 19 Apr 2024 09:57:51 +1000 Subject: [PATCH 01/26] Start adding rust backed config request --- Cargo.lock | 79 +++++++-- crates/node-bindings/Cargo.toml | 4 + crates/node-bindings/src/core/mod.rs | 4 + .../src/core/requests/config_request/mod.rs | 1 + crates/node-bindings/src/core/requests/mod.rs | 153 ++++++++++++++++++ .../requests/request_api/js_request_api.rs | 63 ++++++++ .../requests/request_api/mock_request_api.rs | 46 ++++++ .../src/core/requests/request_api/mod.rs | 30 ++++ crates/node-bindings/src/lib.rs | 1 + crates/node-bindings/src/resolver.rs | 10 +- .../core/core/src/requests/ConfigRequest.js | 6 + 11 files changed, 380 insertions(+), 17 deletions(-) create mode 100644 crates/node-bindings/src/core/mod.rs create mode 100644 crates/node-bindings/src/core/requests/config_request/mod.rs create mode 100644 crates/node-bindings/src/core/requests/mod.rs create mode 100644 crates/node-bindings/src/core/requests/request_api/js_request_api.rs create mode 100644 crates/node-bindings/src/core/requests/request_api/mock_request_api.rs create mode 100644 crates/node-bindings/src/core/requests/request_api/mod.rs diff --git a/Cargo.lock b/Cargo.lock index 15b2c78e4d6..8a6cae0d3c7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1252,6 +1252,9 @@ dependencies = [ "parcel-macros", "parcel-resolver", "rayon", + "serde", + "serde_json", + "toml", "xxhash-rust", ] @@ -1478,9 +1481,9 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.69" +version = "1.0.81" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "134c189feb4956b20f6f547d2cf727d4c0fe06722b20a0eec87ed445a97f92da" +checksum = "3d1597b0c024618f09a9c3b8655b7e430397a36d23fdafec26d6965e9eec3eba" dependencies = [ "unicode-ident", ] @@ -1496,9 +1499,9 @@ dependencies = [ [[package]] name = "quote" -version = "1.0.29" +version = "1.0.36" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "573015e8ab27661678357f27dc26460738fd2b6c86e46f386fde94cb5d913105" +checksum = "0fa76aaf39101c457836aec0ce2316dbdc3ab723cdda1c6bd4e6ad4208acaca7" dependencies = [ "proc-macro2", ] @@ -1713,9 +1716,9 @@ checksum = "388a1df253eca08550bef6c72392cfe7c30914bf41df5269b68cbd6ff8f570a3" [[package]] name = "serde" -version = "1.0.189" +version = "1.0.198" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8e422a44e74ad4001bdc8eede9a4570ab52f71190e9c076d14369f38b9200537" +checksum = "9846a40c979031340571da2545a4e5b7c4163bdae79b301d5f86d03979451fcc" dependencies = [ "serde_derive", ] @@ -1731,9 +1734,9 @@ dependencies = [ [[package]] name = "serde_derive" -version = "1.0.189" +version = "1.0.198" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e48d1f918009ce3145511378cf68d613e3b3d9137d67272562080d68a2b32d5" +checksum = "e88edab869b01783ba905e7d0153f9fc1a6505a96e4ad3018011eedb838566d9" dependencies = [ "proc-macro2", "quote", @@ -1742,15 +1745,24 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.91" +version = "1.0.116" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "877c235533714907a8c2464236f5c4b2a17262ef1bd71f38f35ea592c8da6883" +checksum = "3e17db7126d17feb94eb3fad46bf1a96b034e8aacbc2e775fe81505f8b0b2813" dependencies = [ "itoa", "ryu", "serde", ] +[[package]] +name = "serde_spanned" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb3622f419d1296904700073ea6cc23ad690adbd66f13ea683df73298736f0c1" +dependencies = [ + "serde", +] + [[package]] name = "sha-1" version = "0.10.0" @@ -2589,9 +2601,9 @@ dependencies = [ [[package]] name = "syn" -version = "2.0.38" +version = "2.0.60" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e96b79aaa137db8f61e26363a0c9b47d8b4ec75da28b7d1d614c2303e232408b" +checksum = "909518bc7b1c9b779f1bbf07f2929d35af9f0f37e47c6e9ef7f9dddc1e1821f3" dependencies = [ "proc-macro2", "quote", @@ -2682,6 +2694,40 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cda74da7e1a664f795bb1f8a87ec406fb89a02522cf6e50620d016add6dbbf5c" +[[package]] +name = "toml" +version = "0.8.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e9dd1545e8208b4a5af1aa9bbd0b4cf7e9ea08fabc5d0a5c67fcaafa17433aa3" +dependencies = [ + "serde", + "serde_spanned", + "toml_datetime", + "toml_edit", +] + +[[package]] +name = "toml_datetime" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3550f4e9685620ac18a50ed434eb3aec30db8ba93b0287467bca5826ea25baf1" +dependencies = [ + "serde", +] + +[[package]] +name = "toml_edit" +version = "0.22.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e40bb779c5187258fd7aad0eb68cb8706a0a81fa712fbea808ab43c4b8374c4" +dependencies = [ + "indexmap 2.1.0", + "serde", + "serde_spanned", + "toml_datetime", + "winnow", +] + [[package]] name = "tracing" version = "0.1.40" @@ -2983,6 +3029,15 @@ version = "0.48.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed94fce61571a4006852b7389a063ab983c02eb1bb37b47f8272ce92d06d9538" +[[package]] +name = "winnow" +version = "0.6.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f0c976aaaa0e1f90dbb21e9587cdaf1d9679a1cde8875c0d6bd83ab96a208352" +dependencies = [ + "memchr", +] + [[package]] name = "wyz" version = "0.5.1" diff --git a/crates/node-bindings/Cargo.toml b/crates/node-bindings/Cargo.toml index a55a2b3fe1d..217a67955c6 100644 --- a/crates/node-bindings/Cargo.toml +++ b/crates/node-bindings/Cargo.toml @@ -14,6 +14,10 @@ parcel-resolver = { path = "../../packages/utils/node-resolver-rs" } dashmap = "5.4.0" xxhash-rust = { version = "0.8.2", features = ["xxh3"] } +serde = "1.0.198" +serde_json = "1.0.116" +toml = "0.8.12" + [target.'cfg(not(target_arch = "wasm32"))'.dependencies] napi = {version = "2.12.6", features = ["serde-json", "napi4", "napi5"]} parcel-dev-dep-resolver = { path = "../../packages/utils/dev-dep-resolver" } diff --git a/crates/node-bindings/src/core/mod.rs b/crates/node-bindings/src/core/mod.rs new file mode 100644 index 00000000000..9a926cd401d --- /dev/null +++ b/crates/node-bindings/src/core/mod.rs @@ -0,0 +1,4 @@ +//! Core re-implementation in Rust + +/// Request types and run functions +mod requests; diff --git a/crates/node-bindings/src/core/requests/config_request/mod.rs b/crates/node-bindings/src/core/requests/config_request/mod.rs new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/crates/node-bindings/src/core/requests/config_request/mod.rs @@ -0,0 +1 @@ + diff --git a/crates/node-bindings/src/core/requests/mod.rs b/crates/node-bindings/src/core/requests/mod.rs new file mode 100644 index 00000000000..8fd9dcf4847 --- /dev/null +++ b/crates/node-bindings/src/core/requests/mod.rs @@ -0,0 +1,153 @@ +use std::path::Path; + +use napi::bindgen_prelude::{FromNapiValue, ToNapiValue}; +use napi::sys::{napi_env, napi_value}; +use napi::{Env, JsFunction, JsObject, JsUnknown, NapiRaw}; +use napi_derive::napi; + +use crate::core::requests::request_api::js_request_api::JSRequestApi; +use crate::resolver::JsFileSystem; +use parcel_resolver::FileSystem; +use request_api::RequestApi; + +mod config_request; +mod request_api; + +/// Cast method field to function +/// +/// # Safety +/// Uses raw NAPI casts, but checks that object field is a function +pub fn get_function(env: Env, js_object: &JsObject, field_name: &str) -> napi::Result { + let Some(method): Option = js_object.get(field_name)? else { + return Err(napi::Error::from_reason("Method not found")); + }; + let function_class: JsUnknown = env.get_global()?.get_named_property("Function")?; + let is_function = method.instanceof(function_class)?; + if !is_function { + return Err(napi::Error::from_reason("Method is not a function")); + } + + let method_fn = unsafe { JsFunction::from_napi_value(env.raw(), method.raw()) }?; + Ok(method_fn) +} + +pub type ProjectPath = String; + +pub type InternalGlob = String; + +#[napi(object)] +pub struct ConfigKeyChange { + pub file_path: ProjectPath, + pub config_key: String, +} + +#[napi(object)] +pub struct InternalFileInvalidation { + pub file_path: ProjectPath, +} + +#[napi(object)] +pub struct InternalGlobInvalidation { + pub glob: InternalGlob, +} + +#[napi(object)] +pub struct InternalFileAboveInvalidation { + pub file_name: String, + pub above_file_path: ProjectPath, +} + +#[napi(object)] +pub struct InternalFileCreateInvalidation { + pub file: Option, + pub glob: Option, + pub file_above: Option, +} + +#[napi(object)] +pub struct ConfigRequest { + pub id: String, + // Set<...> + pub invalidate_on_file_change: Vec, + pub invalidate_on_config_key_change: Vec, + pub invalidate_on_file_create: Vec, + // Set<...> + pub invalidate_on_env_change: Vec, + // Set<...> + pub invalidate_on_option_change: Vec, + pub invalidate_on_startup: bool, + pub invalidate_on_build: bool, +} + +fn get_config_key_content_hash( + file_path: &str, + config_key: &str, + input_fs: &impl FileSystem, + project_root: &str, +) -> napi::Result { + todo!("") +} + +fn run_config_request( + config_request: &ConfigRequest, + api: impl RequestApi, + input_fs: &impl FileSystem, + project_root: &str, +) -> napi::Result<()> { + for file_path in &config_request.invalidate_on_file_change { + api.invalidate_on_file_update(Path::new(file_path))?; + } + + for config_key_change in &config_request.invalidate_on_config_key_change { + let content_hash = get_config_key_content_hash( + &config_key_change.file_path, + &config_key_change.config_key, + input_fs, + &project_root, + )?; + api.invalidate_on_config_key_change( + Path::new(&config_key_change.file_path), + &config_key_change.config_key, + &content_hash, + )?; + } + + Ok(()) +} + +#[napi] +fn run_config_request_js( + env: Env, + config_request: ConfigRequest, + api: JsObject, + options: JsObject, +) -> napi::Result<()> { + let api = JSRequestApi::new(env, api); + let input_fs: JsFileSystem = todo!(""); + + run_config_request(&config_request, api, &input_fs, todo!("")) +} + +#[napi(object)] +struct RequestOptions {} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_execute() { + let config_request = ConfigRequest { + id: "".to_string(), + invalidate_on_file_change: vec![], + invalidate_on_config_key_change: vec![], + invalidate_on_file_create: vec![], + invalidate_on_env_change: vec![], + invalidate_on_option_change: vec![], + invalidate_on_startup: false, + invalidate_on_build: false, + }; + + run_config_request(&config_request, todo!(""), todo!(""), todo!("")).unwrap() + } +} diff --git a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs new file mode 100644 index 00000000000..8e9c5d71f87 --- /dev/null +++ b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs @@ -0,0 +1,63 @@ +use std::path::Path; + +use napi::{Env, JsObject}; + +use crate::core::requests; +use crate::core::requests::request_api::RequestApi; + +pub struct JSRequestApi { + env: Env, + js_object: JsObject, +} + +impl JSRequestApi { + pub fn new(env: Env, js_object: JsObject) -> Self { + Self { env, js_object } + } +} + +impl RequestApi for JSRequestApi { + fn invalidate_on_file_update(&self, path: &Path) -> napi::Result<()> { + let field_name = "invalidateOnFileUpdate"; + let method_fn = requests::get_function(self.env, &self.js_object, field_name)?; + let path_js_string = self.env.create_string(path.to_str().unwrap())?; + method_fn.call(Some(&self.js_object), &[&path_js_string])?; + + Ok(()) + } + + fn invalidate_on_file_delete(&self, path: &Path) -> napi::Result<()> { + // ... + Ok(()) + } + + fn invalidate_on_file_create(&self, path: &Path) -> napi::Result<()> { + // ... + Ok(()) + } + + fn invalidate_on_config_key_change( + &self, + file_path: &Path, + config_key: &str, + content_hash: &str, + ) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_startup(&self, env: Env) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_build(&self, env: Env) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_env_change(&self, env_change: &str) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_option_change(&self, option: &str) -> napi::Result<()> { + Ok(()) + } +} diff --git a/crates/node-bindings/src/core/requests/request_api/mock_request_api.rs b/crates/node-bindings/src/core/requests/request_api/mock_request_api.rs new file mode 100644 index 00000000000..913597c3275 --- /dev/null +++ b/crates/node-bindings/src/core/requests/request_api/mock_request_api.rs @@ -0,0 +1,46 @@ +use std::path::Path; + +use napi::Env; + +use crate::core::requests::request_api::RequestApi; + +pub struct MockRequestApi; + +impl RequestApi for MockRequestApi { + fn invalidate_on_file_update(&self, path: &Path) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_file_delete(&self, path: &Path) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_file_create(&self, path: &Path) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_config_key_change( + &self, + file_path: &Path, + config_key: &str, + content_hash: &str, + ) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_startup(&self, env: Env) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_build(&self, env: Env) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_env_change(&self, env_change: &str) -> napi::Result<()> { + Ok(()) + } + + fn invalidate_on_option_change(&self, option: &str) -> napi::Result<()> { + Ok(()) + } +} diff --git a/crates/node-bindings/src/core/requests/request_api/mod.rs b/crates/node-bindings/src/core/requests/request_api/mod.rs new file mode 100644 index 00000000000..2f093e070a7 --- /dev/null +++ b/crates/node-bindings/src/core/requests/request_api/mod.rs @@ -0,0 +1,30 @@ +use std::path::Path; + +use napi::Env; + +pub mod js_request_api; +pub mod mock_request_api; + +pub trait RequestApi { + fn invalidate_on_file_update(&self, path: &Path) -> napi::Result<()>; + fn invalidate_on_file_delete(&self, path: &Path) -> napi::Result<()>; + fn invalidate_on_file_create(&self, path: &Path) -> napi::Result<()>; + fn invalidate_on_config_key_change( + &self, + file_path: &Path, + config_key: &str, + content_hash: &str, + ) -> napi::Result<()>; + fn invalidate_on_startup(&self, env: Env) -> napi::Result<()>; + fn invalidate_on_build(&self, env: Env) -> napi::Result<()>; + fn invalidate_on_env_change(&self, env_change: &str) -> napi::Result<()>; + fn invalidate_on_option_change(&self, option: &str) -> napi::Result<()>; + // fn getInvalidations() -> Vec; + // fn store_result(result: RequestResult, cacheKey: &str); + // fn get_request_result(contentKey: &str); + // fn getPreviousResult(ifMatch: string); + // fn getSubRequests() -> Vec; + // fn getInvalidSubRequests() -> Vec; + // fn canSkipSubrequest(content_key: &str) -> bool; + // fn runRequest(subRequest: Request, opts?: RunRequestOpts, ) => Promise, +} diff --git a/crates/node-bindings/src/lib.rs b/crates/node-bindings/src/lib.rs index e79c2e768df..741dbe6602b 100644 --- a/crates/node-bindings/src/lib.rs +++ b/crates/node-bindings/src/lib.rs @@ -11,6 +11,7 @@ static GLOBAL: jemallocator::Jemalloc = jemallocator::Jemalloc; #[global_allocator] static ALLOC: mimalloc::MiMalloc = mimalloc::MiMalloc; +mod core; #[cfg(not(target_arch = "wasm32"))] mod fs_search; mod hash; diff --git a/crates/node-bindings/src/resolver.rs b/crates/node-bindings/src/resolver.rs index 52918da1eb1..7a4635804a7 100644 --- a/crates/node-bindings/src/resolver.rs +++ b/crates/node-bindings/src/resolver.rs @@ -72,11 +72,11 @@ impl Drop for FunctionRef { } } -struct JsFileSystem { - canonicalize: FunctionRef, - read: FunctionRef, - is_file: FunctionRef, - is_dir: FunctionRef, +pub struct JsFileSystem { + pub canonicalize: FunctionRef, + pub read: FunctionRef, + pub is_file: FunctionRef, + pub is_dir: FunctionRef, } impl FileSystem for JsFileSystem { diff --git a/packages/core/core/src/requests/ConfigRequest.js b/packages/core/core/src/requests/ConfigRequest.js index de41ce0140c..a77f81181af 100644 --- a/packages/core/core/src/requests/ConfigRequest.js +++ b/packages/core/core/src/requests/ConfigRequest.js @@ -17,6 +17,7 @@ import type { import type {LoadedPlugin} from '../ParcelConfig'; import type {RunAPI} from '../RequestTracker'; import type {ProjectPath} from '../projectPath'; +import {runConfigRequestJs as rustRunConfigRequest} from '@parcel/rust'; import {serializeRaw} from '../serializer.js'; import {PluginLogger} from '@parcel/logger'; @@ -30,6 +31,7 @@ import {PluginTracer} from '@parcel/profiler'; import {requestTypes} from '../RequestTracker'; import {fromProjectPath, fromProjectPathRelative} from '../projectPath'; import {createBuildCache} from '../buildCache'; +import {getFeatureFlag} from '@parcel/feature-flags'; export type PluginWithLoadConfig = { loadConfig?: ({| @@ -171,6 +173,10 @@ export async function runConfigRequest( id: 'config_request:' + configRequest.id, type: requestTypes.config_request, run: async ({api, options}) => { + if (true || getFeatureFlag('parcel-v3')) { + return rustRunConfigRequest(configRequest, api, options); + } + for (let filePath of invalidateOnFileChange) { api.invalidateOnFileUpdate(filePath); api.invalidateOnFileDelete(filePath); From 1e3b9a231e99fbc0e082f2de0b802071cc9ee2a7 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Fri, 19 Apr 2024 10:37:52 +1000 Subject: [PATCH 02/26] Start to add tests and use-case --- .mocharc.json | 2 +- Cargo.lock | 45 +++++++++- crates/node-bindings/Cargo.toml | 4 + crates/node-bindings/src/core/requests/mod.rs | 31 ++++--- .../requests/request_api/js_request_api.rs | 9 +- .../requests/request_api/mock_request_api.rs | 46 ---------- .../src/core/requests/request_api/mod.rs | 32 +++++-- crates/node-bindings/src/resolver.rs | 2 +- .../core/core/src/requests/ConfigRequest.js | 6 +- .../core/test/requests/ConfigRequest.test.js | 85 +++++++++++++++++++ packages/core/feature-flags/src/index.js | 1 + packages/core/feature-flags/src/types.js | 4 + 12 files changed, 192 insertions(+), 75 deletions(-) delete mode 100644 crates/node-bindings/src/core/requests/request_api/mock_request_api.rs create mode 100644 packages/core/core/test/requests/ConfigRequest.test.js diff --git a/.mocharc.json b/.mocharc.json index 5b18837e463..090b32f352b 100644 --- a/.mocharc.json +++ b/.mocharc.json @@ -1,5 +1,5 @@ { - "spec": "packages/*/!(integration-tests)/test/*.js", + "spec": "packages/*/!(integration-tests)/test/{*.js,**/*.{test,spec}.js}", "require": ["@parcel/babel-register", "@parcel/test-utils/src/mochaSetup.js"], // TODO: Remove this when https://github.com/nodejs/node/pull/28788 is resolved "exit": true diff --git a/Cargo.lock b/Cargo.lock index 8a6cae0d3c7..120c2480495 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -64,9 +64,9 @@ checksum = "7079075b41f533b8c61d2a4d073c4676e1f8b249ff94a393b0595db304e0dd87" [[package]] name = "anyhow" -version = "1.0.75" +version = "1.0.82" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4668cab20f66d8d020e1fbc0ebe47217433c1b6c8f2040faf858554e394ace6" +checksum = "f538837af36e6f6a9be0faa67f9a314f8119e4e4b5867c6ab40ed60360142519" [[package]] name = "arrayvec" @@ -473,6 +473,12 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fea41bba32d969b513997752735605054bc0dfa92b4c56bf1189f2e174be7a10" +[[package]] +name = "downcast" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" + [[package]] name = "dunce" version = "1.0.3" @@ -568,6 +574,12 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fragile" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c2141d6d6c8512188a7891b4b01590a45f6dac67afb4f255c4124dbb86d4eaa" + [[package]] name = "from_variant" version = "0.1.7" @@ -1005,6 +1017,33 @@ dependencies = [ "adler", ] +[[package]] +name = "mockall" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "43766c2b5203b10de348ffe19f7e54564b64f3d6018ff7648d1e2d6d3a0f0a48" +dependencies = [ + "cfg-if", + "downcast", + "fragile", + "lazy_static", + "mockall_derive", + "predicates", + "predicates-tree", +] + +[[package]] +name = "mockall_derive" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af7cbce79ec385a1d4f54baa90a76401eb15d9cab93685f62e7e9f942aa00ae2" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "mozjpeg-sys" version = "1.0.3" @@ -1235,6 +1274,7 @@ dependencies = [ name = "parcel-node-bindings" version = "0.1.0" dependencies = [ + "anyhow", "crossbeam-channel", "dashmap", "getrandom", @@ -1242,6 +1282,7 @@ dependencies = [ "jemallocator", "libc", "mimalloc", + "mockall", "mozjpeg-sys", "napi", "napi-build", diff --git a/crates/node-bindings/Cargo.toml b/crates/node-bindings/Cargo.toml index 217a67955c6..11e408f8ea8 100644 --- a/crates/node-bindings/Cargo.toml +++ b/crates/node-bindings/Cargo.toml @@ -17,6 +17,8 @@ xxhash-rust = { version = "0.8.2", features = ["xxh3"] } serde = "1.0.198" serde_json = "1.0.116" toml = "0.8.12" +anyhow = "1.0.82" +mockall = "0.12.1" [target.'cfg(not(target_arch = "wasm32"))'.dependencies] napi = {version = "2.12.6", features = ["serde-json", "napi4", "napi5"]} @@ -39,5 +41,7 @@ jemallocator = { version = "0.3.2", features = ["disable_initial_exec_tls"] } [target.'cfg(windows)'.dependencies] mimalloc = { version = "0.1.25", default-features = false } +[dev-dependencies] + [build-dependencies] napi-build = "2" diff --git a/crates/node-bindings/src/core/requests/mod.rs b/crates/node-bindings/src/core/requests/mod.rs index 8fd9dcf4847..703e2b253b9 100644 --- a/crates/node-bindings/src/core/requests/mod.rs +++ b/crates/node-bindings/src/core/requests/mod.rs @@ -1,23 +1,25 @@ use std::path::Path; +use std::rc::Rc; use napi::bindgen_prelude::{FromNapiValue, ToNapiValue}; -use napi::sys::{napi_env, napi_value}; use napi::{Env, JsFunction, JsObject, JsUnknown, NapiRaw}; use napi_derive::napi; -use crate::core::requests::request_api::js_request_api::JSRequestApi; -use crate::resolver::JsFileSystem; use parcel_resolver::FileSystem; use request_api::RequestApi; +use crate::core::requests::request_api::js_request_api::JSRequestApi; +use crate::resolver::JsFileSystem; + mod config_request; mod request_api; -/// Cast method field to function +/// Get an object field as a JSFunction. Will error out if the field is not present or isn't an +/// instance of the global `"Function"`. /// /// # Safety /// Uses raw NAPI casts, but checks that object field is a function -pub fn get_function(env: Env, js_object: &JsObject, field_name: &str) -> napi::Result { +pub fn get_function(env: &Env, js_object: &JsObject, field_name: &str) -> napi::Result { let Some(method): Option = js_object.get(field_name)? else { return Err(napi::Error::from_reason("Method not found")); }; @@ -90,7 +92,7 @@ fn get_config_key_content_hash( fn run_config_request( config_request: &ConfigRequest, - api: impl RequestApi, + api: &impl RequestApi, input_fs: &impl FileSystem, project_root: &str, ) -> napi::Result<()> { @@ -116,16 +118,19 @@ fn run_config_request( } #[napi] -fn run_config_request_js( +fn napi_run_config_request( env: Env, config_request: ConfigRequest, api: JsObject, - options: JsObject, + _options: JsObject, ) -> napi::Result<()> { + // Technically we could move `env` to JSRequestAPI but in order to + // be able to use env on more places we rc it. + let env = Rc::new(env); let api = JSRequestApi::new(env, api); let input_fs: JsFileSystem = todo!(""); - run_config_request(&config_request, api, &input_fs, todo!("")) + run_config_request(&config_request, &api, &input_fs, todo!("")) } #[napi(object)] @@ -133,6 +138,9 @@ struct RequestOptions {} #[cfg(test)] mod test { + use crate::core::requests::request_api::MockRequestApi; + use parcel_resolver::OsFileSystem; + use super::*; #[test] @@ -147,7 +155,10 @@ mod test { invalidate_on_startup: false, invalidate_on_build: false, }; + let request_api = MockRequestApi::new(); + let file_system = OsFileSystem::default(); + let project_root = ""; - run_config_request(&config_request, todo!(""), todo!(""), todo!("")).unwrap() + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap() } } diff --git a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs index 8e9c5d71f87..46ae216ead2 100644 --- a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs +++ b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs @@ -1,17 +1,20 @@ use std::path::Path; +use std::rc::Rc; use napi::{Env, JsObject}; +use requests::get_function; use crate::core::requests; use crate::core::requests::request_api::RequestApi; pub struct JSRequestApi { - env: Env, + // TODO: Make sure it is safe to hold the environment like this + env: Rc, js_object: JsObject, } impl JSRequestApi { - pub fn new(env: Env, js_object: JsObject) -> Self { + pub fn new(env: Rc, js_object: JsObject) -> Self { Self { env, js_object } } } @@ -19,7 +22,7 @@ impl JSRequestApi { impl RequestApi for JSRequestApi { fn invalidate_on_file_update(&self, path: &Path) -> napi::Result<()> { let field_name = "invalidateOnFileUpdate"; - let method_fn = requests::get_function(self.env, &self.js_object, field_name)?; + let method_fn = get_function(&self.env, &self.js_object, field_name)?; let path_js_string = self.env.create_string(path.to_str().unwrap())?; method_fn.call(Some(&self.js_object), &[&path_js_string])?; diff --git a/crates/node-bindings/src/core/requests/request_api/mock_request_api.rs b/crates/node-bindings/src/core/requests/request_api/mock_request_api.rs deleted file mode 100644 index 913597c3275..00000000000 --- a/crates/node-bindings/src/core/requests/request_api/mock_request_api.rs +++ /dev/null @@ -1,46 +0,0 @@ -use std::path::Path; - -use napi::Env; - -use crate::core::requests::request_api::RequestApi; - -pub struct MockRequestApi; - -impl RequestApi for MockRequestApi { - fn invalidate_on_file_update(&self, path: &Path) -> napi::Result<()> { - Ok(()) - } - - fn invalidate_on_file_delete(&self, path: &Path) -> napi::Result<()> { - Ok(()) - } - - fn invalidate_on_file_create(&self, path: &Path) -> napi::Result<()> { - Ok(()) - } - - fn invalidate_on_config_key_change( - &self, - file_path: &Path, - config_key: &str, - content_hash: &str, - ) -> napi::Result<()> { - Ok(()) - } - - fn invalidate_on_startup(&self, env: Env) -> napi::Result<()> { - Ok(()) - } - - fn invalidate_on_build(&self, env: Env) -> napi::Result<()> { - Ok(()) - } - - fn invalidate_on_env_change(&self, env_change: &str) -> napi::Result<()> { - Ok(()) - } - - fn invalidate_on_option_change(&self, option: &str) -> napi::Result<()> { - Ok(()) - } -} diff --git a/crates/node-bindings/src/core/requests/request_api/mod.rs b/crates/node-bindings/src/core/requests/request_api/mod.rs index 2f093e070a7..078cf8de251 100644 --- a/crates/node-bindings/src/core/requests/request_api/mod.rs +++ b/crates/node-bindings/src/core/requests/request_api/mod.rs @@ -1,24 +1,38 @@ +use mockall::automock; use std::path::Path; use napi::Env; pub mod js_request_api; -pub mod mock_request_api; +// TODO: Move this into an associated type of the struct +pub type RequestApiResult = napi::Result; + +#[automock] pub trait RequestApi { - fn invalidate_on_file_update(&self, path: &Path) -> napi::Result<()>; - fn invalidate_on_file_delete(&self, path: &Path) -> napi::Result<()>; - fn invalidate_on_file_create(&self, path: &Path) -> napi::Result<()>; + /// Invalidate the current request when a file at `path` is updated + fn invalidate_on_file_update(&self, path: &Path) -> RequestApiResult<()>; + /// Invalidate the current request when a file at `path` is deleted + fn invalidate_on_file_delete(&self, path: &Path) -> RequestApiResult<()>; + /// Invalidate the current request when a file at `path` is created + fn invalidate_on_file_create(&self, path: &Path) -> RequestApiResult<()>; + /// Invalidate the current request when a config key from the configuration + /// file at path is changed fn invalidate_on_config_key_change( &self, file_path: &Path, config_key: &str, content_hash: &str, - ) -> napi::Result<()>; - fn invalidate_on_startup(&self, env: Env) -> napi::Result<()>; - fn invalidate_on_build(&self, env: Env) -> napi::Result<()>; - fn invalidate_on_env_change(&self, env_change: &str) -> napi::Result<()>; - fn invalidate_on_option_change(&self, option: &str) -> napi::Result<()>; + ) -> RequestApiResult<()>; + /// Invalidate the current request on start-up + fn invalidate_on_startup(&self, env: Env) -> RequestApiResult<()>; + /// Invalidate the current request on builds + fn invalidate_on_build(&self, env: Env) -> RequestApiResult<()>; + /// Invalidate the current request on environment variable changes + fn invalidate_on_env_change(&self, env_change: &str) -> RequestApiResult<()>; + /// Invalidate the current request on option changes + fn invalidate_on_option_change(&self, option: &str) -> RequestApiResult<()>; + // fn getInvalidations() -> Vec; // fn store_result(result: RequestResult, cacheKey: &str); // fn get_request_result(contentKey: &str); diff --git a/crates/node-bindings/src/resolver.rs b/crates/node-bindings/src/resolver.rs index 7a4635804a7..d114cfdeaf3 100644 --- a/crates/node-bindings/src/resolver.rs +++ b/crates/node-bindings/src/resolver.rs @@ -44,7 +44,7 @@ pub struct JsResolverOptions { pub typescript: Option, } -struct FunctionRef { +pub struct FunctionRef { env: Env, reference: Ref<()>, } diff --git a/packages/core/core/src/requests/ConfigRequest.js b/packages/core/core/src/requests/ConfigRequest.js index a77f81181af..46c92d50522 100644 --- a/packages/core/core/src/requests/ConfigRequest.js +++ b/packages/core/core/src/requests/ConfigRequest.js @@ -17,7 +17,7 @@ import type { import type {LoadedPlugin} from '../ParcelConfig'; import type {RunAPI} from '../RequestTracker'; import type {ProjectPath} from '../projectPath'; -import {runConfigRequestJs as rustRunConfigRequest} from '@parcel/rust'; +import {napiRunConfigRequest} from '@parcel/rust'; import {serializeRaw} from '../serializer.js'; import {PluginLogger} from '@parcel/logger'; @@ -173,8 +173,8 @@ export async function runConfigRequest( id: 'config_request:' + configRequest.id, type: requestTypes.config_request, run: async ({api, options}) => { - if (true || getFeatureFlag('parcel-v3')) { - return rustRunConfigRequest(configRequest, api, options); + if (getFeatureFlag('parcelV3')) { + return napiRunConfigRequest(configRequest, api, options); } for (let filePath of invalidateOnFileChange) { diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js new file mode 100644 index 00000000000..fe4009ab99b --- /dev/null +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -0,0 +1,85 @@ +// @flow strict-local + +import {setFeatureFlags, DEFAULT_FEATURE_FLAGS} from '@parcel/feature-flags'; +import assert from 'assert'; + +import {runConfigRequest} from '../../src/requests/ConfigRequest'; +import type {RunAPI} from '../../src/RequestTracker'; +import sinon from 'sinon'; +import {toProjectPath} from '../../src/projectPath'; + +// $FlowFixMe unclear-type forgive me +const mockCast = (f: any): any => f; + +describe('ConfigRequest tests', () => { + const projectRoot = ''; + const mockRunApi: RunAPI = { + storeResult: sinon.spy(), + canSkipSubrequest: sinon.spy(), + invalidateOnFileCreate: sinon.spy(), + getInvalidSubRequests: sinon.spy(), + getInvalidations: sinon.spy(), + getPreviousResult: sinon.spy(), + getRequestResult: sinon.spy(), + getSubRequests: sinon.spy(), + invalidateOnBuild: sinon.spy(), + invalidateOnConfigKeyChange: sinon.spy(), + invalidateOnEnvChange: sinon.spy(), + invalidateOnFileDelete: sinon.spy(), + invalidateOnFileUpdate: sinon.spy(), + invalidateOnOptionChange: sinon.spy(), + invalidateOnStartup: sinon.spy(), + runRequest: sinon.spy(request => { + request.run({api: mockRunApi, options: {}}); + }), + }; + + ['rust', 'js'].forEach(backend => { + describe(`${backend} backed`, () => { + beforeEach(() => { + setFeatureFlags({ + ...DEFAULT_FEATURE_FLAGS, + parcelV3: backend === 'rust', + }); + }); + + it('can execute a config request', async () => { + await runConfigRequest(mockRunApi, { + id: 'config_request_test', + invalidateOnBuild: false, + invalidateOnConfigKeyChange: [], + invalidateOnFileCreate: [], + invalidateOnEnvChange: new Set(), + invalidateOnOptionChange: new Set(), + invalidateOnStartup: false, + invalidateOnFileChange: new Set(), + }); + }); + + it('forwards "invalidateOnFileChange" calls to runAPI', async () => { + await runConfigRequest(mockRunApi, { + id: 'config_request_test', + invalidateOnBuild: false, + invalidateOnConfigKeyChange: [], + invalidateOnFileCreate: [], + invalidateOnEnvChange: new Set(), + invalidateOnOptionChange: new Set(), + invalidateOnStartup: false, + invalidateOnFileChange: new Set([ + toProjectPath(projectRoot, 'path1'), + toProjectPath(projectRoot, 'path2'), + ]), + }); + + assert( + mockCast(mockRunApi.invalidateOnFileUpdate).called, + 'Invalidate was called', + ); + assert( + mockCast(mockRunApi.invalidateOnFileUpdate).calledWith('path1'), + 'Invalidate was called with path1', + ); + }); + }); + }); +}); diff --git a/packages/core/feature-flags/src/index.js b/packages/core/feature-flags/src/index.js index c66b372d0dd..59344a04fd3 100644 --- a/packages/core/feature-flags/src/index.js +++ b/packages/core/feature-flags/src/index.js @@ -8,6 +8,7 @@ export type FeatureFlags = _FeatureFlags; export const DEFAULT_FEATURE_FLAGS: FeatureFlags = { exampleFeature: false, configKeyInvalidation: false, + parcelV3: false, }; let featureFlagValues: FeatureFlags = {...DEFAULT_FEATURE_FLAGS}; diff --git a/packages/core/feature-flags/src/types.js b/packages/core/feature-flags/src/types.js index 4edb1cf42f0..bed8d10b9e0 100644 --- a/packages/core/feature-flags/src/types.js +++ b/packages/core/feature-flags/src/types.js @@ -9,4 +9,8 @@ export type FeatureFlags = {| * `config.getConfigFrom(..., {packageKey: '...'})` and the value itself hasn't changed. */ +configKeyInvalidation: boolean, + /** + * Rust backed requests + */ + +parcelV3: boolean, |}; From 6257ea095c3ddfb02af17d7869783457effc899d Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Fri, 19 Apr 2024 11:09:12 +1000 Subject: [PATCH 03/26] Add tests for config request --- .../src/core/requests/config_request/mod.rs | 152 ++++++++++++++++ crates/node-bindings/src/core/requests/mod.rs | 165 +++++------------- .../requests/request_api/js_request_api.rs | 76 ++++++-- .../src/core/requests/request_api/mod.rs | 4 +- .../core/core/src/requests/ConfigRequest.js | 22 ++- .../core/test/requests/ConfigRequest.test.js | 77 ++++++-- 6 files changed, 337 insertions(+), 159 deletions(-) diff --git a/crates/node-bindings/src/core/requests/config_request/mod.rs b/crates/node-bindings/src/core/requests/config_request/mod.rs index 8b137891791..b623a85685c 100644 --- a/crates/node-bindings/src/core/requests/config_request/mod.rs +++ b/crates/node-bindings/src/core/requests/config_request/mod.rs @@ -1 +1,153 @@ +use std::path::Path; +use napi_derive::napi; + +use parcel_resolver::FileSystem; + +use crate::core::requests::request_api::RequestApi; + +pub type ProjectPath = String; + +pub type InternalGlob = String; + +#[napi(object)] +pub struct ConfigKeyChange { + pub file_path: ProjectPath, + pub config_key: String, +} + +#[napi(object)] +pub struct InternalFileInvalidation { + pub file_path: ProjectPath, +} + +#[napi(object)] +pub struct InternalGlobInvalidation { + pub glob: InternalGlob, +} + +#[napi(object)] +pub struct InternalFileAboveInvalidation { + pub file_name: String, + pub above_file_path: ProjectPath, +} + +#[napi(object)] +pub struct InternalFileCreateInvalidation { + pub file: Option, + pub glob: Option, + pub file_above: Option, +} + +#[napi(object)] +pub struct ConfigRequest { + pub id: String, + // Set<...> + pub invalidate_on_file_change: Vec, + pub invalidate_on_config_key_change: Vec, + pub invalidate_on_file_create: Vec, + // Set<...> + pub invalidate_on_env_change: Vec, + // Set<...> + pub invalidate_on_option_change: Vec, + pub invalidate_on_startup: bool, + pub invalidate_on_build: bool, +} + +fn get_config_key_content_hash( + file_path: &str, + config_key: &str, + input_fs: &impl FileSystem, + project_root: &str, +) -> napi::Result { + todo!("") +} + +pub fn run_config_request( + config_request: &ConfigRequest, + api: &impl RequestApi, + input_fs: &impl FileSystem, + project_root: &str, +) -> napi::Result<()> { + for file_path in &config_request.invalidate_on_file_change { + let file_path = Path::new(file_path); + api.invalidate_on_file_update(file_path)?; + api.invalidate_on_file_delete(file_path)?; + } + + for config_key_change in &config_request.invalidate_on_config_key_change { + let content_hash = get_config_key_content_hash( + &config_key_change.file_path, + &config_key_change.config_key, + input_fs, + &project_root, + )?; + api.invalidate_on_config_key_change( + Path::new(&config_key_change.file_path), + &config_key_change.config_key, + &content_hash, + )?; + } + + for invalidation in &config_request.invalidate_on_file_create { + if let Some(file) = &invalidation.file { + api.invalidate_on_file_create(Path::new(&file.file_path))?; + } + if let Some(glob) = &invalidation.glob { + api.invalidate_on_file_create(Path::new(&glob.glob))?; + } + if let Some(file_above) = &invalidation.file_above { + api.invalidate_on_file_create(Path::new(&file_above.above_file_path))?; + } + } + + for env in &config_request.invalidate_on_env_change { + api.invalidate_on_env_change(env)?; + } + + for option in &config_request.invalidate_on_option_change { + api.invalidate_on_option_change(option)?; + } + + if config_request.invalidate_on_startup { + api.invalidate_on_startup()?; + } + + if config_request.invalidate_on_build { + api.invalidate_on_build()?; + } + + Ok(()) +} + +#[napi(object)] +struct RequestOptions {} + +#[cfg(test)] +mod test { + use parcel_resolver::OsFileSystem; + + use crate::core::requests::config_request::run_config_request; + use crate::core::requests::request_api::MockRequestApi; + + use super::*; + + #[test] + fn test_run_config_request() { + let config_request = ConfigRequest { + id: "".to_string(), + invalidate_on_file_change: vec![], + invalidate_on_config_key_change: vec![], + invalidate_on_file_create: vec![], + invalidate_on_env_change: vec![], + invalidate_on_option_change: vec![], + invalidate_on_startup: false, + invalidate_on_build: false, + }; + let request_api = MockRequestApi::new(); + let file_system = OsFileSystem::default(); + let project_root = ""; + + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap() + } +} diff --git a/crates/node-bindings/src/core/requests/mod.rs b/crates/node-bindings/src/core/requests/mod.rs index 703e2b253b9..bcea82b22a1 100644 --- a/crates/node-bindings/src/core/requests/mod.rs +++ b/crates/node-bindings/src/core/requests/mod.rs @@ -1,15 +1,14 @@ -use std::path::Path; use std::rc::Rc; use napi::bindgen_prelude::{FromNapiValue, ToNapiValue}; -use napi::{Env, JsFunction, JsObject, JsUnknown, NapiRaw}; +use napi::{Env, JsFunction, JsObject, JsString, JsUnknown, NapiRaw}; use napi_derive::napi; -use parcel_resolver::FileSystem; +use parcel_resolver::{FileSystem, OsFileSystem}; use request_api::RequestApi; +use crate::core::requests::config_request::ConfigRequest; use crate::core::requests::request_api::js_request_api::JSRequestApi; -use crate::resolver::JsFileSystem; mod config_request; mod request_api; @@ -17,103 +16,51 @@ mod request_api; /// Get an object field as a JSFunction. Will error out if the field is not present or isn't an /// instance of the global `"Function"`. /// -/// # Safety +/// ## Safety /// Uses raw NAPI casts, but checks that object field is a function pub fn get_function(env: &Env, js_object: &JsObject, field_name: &str) -> napi::Result { let Some(method): Option = js_object.get(field_name)? else { - return Err(napi::Error::from_reason("Method not found")); + return Err(napi::Error::from_reason("[napi] Method not found")); }; let function_class: JsUnknown = env.get_global()?.get_named_property("Function")?; let is_function = method.instanceof(function_class)?; if !is_function { - return Err(napi::Error::from_reason("Method is not a function")); + return Err(napi::Error::from_reason("[napi] Method is not a function")); } let method_fn = unsafe { JsFunction::from_napi_value(env.raw(), method.raw()) }?; Ok(method_fn) } -pub type ProjectPath = String; - -pub type InternalGlob = String; - -#[napi(object)] -pub struct ConfigKeyChange { - pub file_path: ProjectPath, - pub config_key: String, -} - -#[napi(object)] -pub struct InternalFileInvalidation { - pub file_path: ProjectPath, -} - -#[napi(object)] -pub struct InternalGlobInvalidation { - pub glob: InternalGlob, -} - -#[napi(object)] -pub struct InternalFileAboveInvalidation { - pub file_name: String, - pub above_file_path: ProjectPath, -} - -#[napi(object)] -pub struct InternalFileCreateInvalidation { - pub file: Option, - pub glob: Option, - pub file_above: Option, -} - -#[napi(object)] -pub struct ConfigRequest { - pub id: String, - // Set<...> - pub invalidate_on_file_change: Vec, - pub invalidate_on_config_key_change: Vec, - pub invalidate_on_file_create: Vec, - // Set<...> - pub invalidate_on_env_change: Vec, - // Set<...> - pub invalidate_on_option_change: Vec, - pub invalidate_on_startup: bool, - pub invalidate_on_build: bool, -} - -fn get_config_key_content_hash( - file_path: &str, - config_key: &str, - input_fs: &impl FileSystem, - project_root: &str, -) -> napi::Result { - todo!("") -} - -fn run_config_request( - config_request: &ConfigRequest, - api: &impl RequestApi, - input_fs: &impl FileSystem, - project_root: &str, +/// Call a method on an object with a set of arguments. +/// +/// Will error out if the method doesn't exist or if the field is not a function. +/// +/// This does some redundant work ; so you may want to call `get_function` +/// directly if calling a method on a loop. +/// +/// The function takes `JsUnknown` references so any type can be used as an +/// argument. +/// +/// ## Safety +/// Uses raw NAPI casts, but checks that object field is a function +/// +/// ## Example +/// ```skip +/// let string_parameter = env.create_string(path.to_str().unwrap())?; +/// let args = [&string_parameter.into_unknown()]; +/// let field_name = "method"; +/// +/// call_method(&self.env, &js_object, field_name, &args)?; +/// ``` +pub fn call_method( + env: &Env, + js_object: &JsObject, + field_name: &str, + args: &[&JsUnknown], ) -> napi::Result<()> { - for file_path in &config_request.invalidate_on_file_change { - api.invalidate_on_file_update(Path::new(file_path))?; - } - - for config_key_change in &config_request.invalidate_on_config_key_change { - let content_hash = get_config_key_content_hash( - &config_key_change.file_path, - &config_key_change.config_key, - input_fs, - &project_root, - )?; - api.invalidate_on_config_key_change( - Path::new(&config_key_change.file_path), - &config_key_change.config_key, - &content_hash, - )?; - } - + let method_fn = get_function(env, js_object, field_name)?; + method_fn.call(Some(&js_object), &args)?; Ok(()) } @@ -122,43 +69,21 @@ fn napi_run_config_request( env: Env, config_request: ConfigRequest, api: JsObject, - _options: JsObject, + options: JsObject, ) -> napi::Result<()> { // Technically we could move `env` to JSRequestAPI but in order to // be able to use env on more places we rc it. let env = Rc::new(env); let api = JSRequestApi::new(env, api); - let input_fs: JsFileSystem = todo!(""); - - run_config_request(&config_request, &api, &input_fs, todo!("")) -} - -#[napi(object)] -struct RequestOptions {} - -#[cfg(test)] -mod test { - use crate::core::requests::request_api::MockRequestApi; - use parcel_resolver::OsFileSystem; - - use super::*; - - #[test] - fn test_execute() { - let config_request = ConfigRequest { - id: "".to_string(), - invalidate_on_file_change: vec![], - invalidate_on_config_key_change: vec![], - invalidate_on_file_create: vec![], - invalidate_on_env_change: vec![], - invalidate_on_option_change: vec![], - invalidate_on_startup: false, - invalidate_on_build: false, - }; - let request_api = MockRequestApi::new(); - let file_system = OsFileSystem::default(); - let project_root = ""; + let input_fs = OsFileSystem::default(); + let Some(project_root): Option = options.get("projectRoot")? else { + return Err(napi::Error::from_reason( + "[napi] Missing required projectRoot options field", + )); + }; + // TODO: what if the string is UTF16 or latin? + let project_root = project_root.into_utf8()?; + let project_root = project_root.as_str()?; - run_config_request(&config_request, &request_api, &file_system, project_root).unwrap() - } + config_request::run_config_request(&config_request, &api, &input_fs, project_root) } diff --git a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs index 46ae216ead2..07beadfbc37 100644 --- a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs +++ b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs @@ -2,10 +2,9 @@ use std::path::Path; use std::rc::Rc; use napi::{Env, JsObject}; -use requests::get_function; -use crate::core::requests; -use crate::core::requests::request_api::RequestApi; +use crate::core::requests::call_method; +use crate::core::requests::request_api::{RequestApi, RequestApiResult}; pub struct JSRequestApi { // TODO: Make sure it is safe to hold the environment like this @@ -20,22 +19,36 @@ impl JSRequestApi { } impl RequestApi for JSRequestApi { - fn invalidate_on_file_update(&self, path: &Path) -> napi::Result<()> { - let field_name = "invalidateOnFileUpdate"; - let method_fn = get_function(&self.env, &self.js_object, field_name)?; + fn invalidate_on_file_update(&self, path: &Path) -> RequestApiResult<()> { let path_js_string = self.env.create_string(path.to_str().unwrap())?; - method_fn.call(Some(&self.js_object), &[&path_js_string])?; - + call_method( + &self.env, + &self.js_object, + "invalidateOnFileUpdate", + &[&path_js_string.into_unknown()], + )?; Ok(()) } - fn invalidate_on_file_delete(&self, path: &Path) -> napi::Result<()> { - // ... + fn invalidate_on_file_delete(&self, path: &Path) -> RequestApiResult<()> { + let path_js_string = self.env.create_string(path.to_str().unwrap())?; + call_method( + &self.env, + &self.js_object, + "invalidateOnFileDelete", + &[&path_js_string.into_unknown()], + )?; Ok(()) } - fn invalidate_on_file_create(&self, path: &Path) -> napi::Result<()> { - // ... + fn invalidate_on_file_create(&self, path: &Path) -> RequestApiResult<()> { + let path_js_string = self.env.create_string(path.to_str().unwrap())?; + call_method( + &self.env, + &self.js_object, + "invalidateOnFileCreate", + &[&path_js_string.into_unknown()], + )?; Ok(()) } @@ -44,23 +57,52 @@ impl RequestApi for JSRequestApi { file_path: &Path, config_key: &str, content_hash: &str, - ) -> napi::Result<()> { + ) -> RequestApiResult<()> { + let path_js_string = self.env.create_string(file_path.to_str().unwrap())?; + let config_key_js_string = self.env.create_string(config_key)?; + let content_hash_js_string = self.env.create_string(content_hash)?; + call_method( + &self.env, + &self.js_object, + "invalidateOnConfigKeyChange", + &[ + &path_js_string.into_unknown(), + &config_key_js_string.into_unknown(), + &content_hash_js_string.into_unknown(), + ], + )?; Ok(()) } - fn invalidate_on_startup(&self, env: Env) -> napi::Result<()> { + fn invalidate_on_startup(&self) -> RequestApiResult<()> { + call_method(&self.env, &self.js_object, "invalidateOnStartup", &[])?; Ok(()) } - fn invalidate_on_build(&self, env: Env) -> napi::Result<()> { + fn invalidate_on_build(&self) -> RequestApiResult<()> { + call_method(&self.env, &self.js_object, "invalidateOnBuild", &[])?; Ok(()) } - fn invalidate_on_env_change(&self, env_change: &str) -> napi::Result<()> { + fn invalidate_on_env_change(&self, env_change: &str) -> RequestApiResult<()> { + let env_change_js_string = self.env.create_string(env_change)?; + call_method( + &self.env, + &self.js_object, + "invalidateOnEnvChange", + &[&env_change_js_string.into_unknown()], + )?; Ok(()) } - fn invalidate_on_option_change(&self, option: &str) -> napi::Result<()> { + fn invalidate_on_option_change(&self, option: &str) -> RequestApiResult<()> { + let option_js_string = self.env.create_string(option)?; + call_method( + &self.env, + &self.js_object, + "invalidateOnOptionChange", + &[&option_js_string.into_unknown()], + )?; Ok(()) } } diff --git a/crates/node-bindings/src/core/requests/request_api/mod.rs b/crates/node-bindings/src/core/requests/request_api/mod.rs index 078cf8de251..df4e7d94ae3 100644 --- a/crates/node-bindings/src/core/requests/request_api/mod.rs +++ b/crates/node-bindings/src/core/requests/request_api/mod.rs @@ -25,9 +25,9 @@ pub trait RequestApi { content_hash: &str, ) -> RequestApiResult<()>; /// Invalidate the current request on start-up - fn invalidate_on_startup(&self, env: Env) -> RequestApiResult<()>; + fn invalidate_on_startup(&self) -> RequestApiResult<()>; /// Invalidate the current request on builds - fn invalidate_on_build(&self, env: Env) -> RequestApiResult<()>; + fn invalidate_on_build(&self) -> RequestApiResult<()>; /// Invalidate the current request on environment variable changes fn invalidate_on_env_change(&self, env_change: &str) -> RequestApiResult<()>; /// Invalidate the current request on option changes diff --git a/packages/core/core/src/requests/ConfigRequest.js b/packages/core/core/src/requests/ConfigRequest.js index 46c92d50522..2ffa6c70035 100644 --- a/packages/core/core/src/requests/ConfigRequest.js +++ b/packages/core/core/src/requests/ConfigRequest.js @@ -174,7 +174,27 @@ export async function runConfigRequest( type: requestTypes.config_request, run: async ({api, options}) => { if (getFeatureFlag('parcelV3')) { - return napiRunConfigRequest(configRequest, api, options); + return napiRunConfigRequest( + { + id: configRequest.id, + invalidateOnBuild: configRequest.invalidateOnBuild, + invalidateOnConfigKeyChange: + configRequest.invalidateOnConfigKeyChange, + invalidateOnFileCreate: configRequest.invalidateOnFileCreate, + invalidateOnEnvChange: Array.from( + configRequest.invalidateOnEnvChange, + ), + invalidateOnOptionChange: Array.from( + configRequest.invalidateOnOptionChange, + ), + invalidateOnStartup: configRequest.invalidateOnStartup, + invalidateOnFileChange: Array.from( + configRequest.invalidateOnFileChange, + ), + }, + api, + options, + ); } for (let filePath of invalidateOnFileChange) { diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index fe4009ab99b..f1b345417cd 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -13,25 +13,31 @@ const mockCast = (f: any): any => f; describe('ConfigRequest tests', () => { const projectRoot = ''; - const mockRunApi: RunAPI = { - storeResult: sinon.spy(), - canSkipSubrequest: sinon.spy(), - invalidateOnFileCreate: sinon.spy(), - getInvalidSubRequests: sinon.spy(), - getInvalidations: sinon.spy(), - getPreviousResult: sinon.spy(), - getRequestResult: sinon.spy(), - getSubRequests: sinon.spy(), - invalidateOnBuild: sinon.spy(), - invalidateOnConfigKeyChange: sinon.spy(), - invalidateOnEnvChange: sinon.spy(), - invalidateOnFileDelete: sinon.spy(), - invalidateOnFileUpdate: sinon.spy(), - invalidateOnOptionChange: sinon.spy(), - invalidateOnStartup: sinon.spy(), - runRequest: sinon.spy(request => { - request.run({api: mockRunApi, options: {}}); - }), + const getMockRunApi = (options: mixed = {projectRoot}): RunAPI => { + const mockRunApi = { + storeResult: sinon.spy(), + canSkipSubrequest: sinon.spy(), + invalidateOnFileCreate: sinon.spy(), + getInvalidSubRequests: sinon.spy(), + getInvalidations: sinon.spy(), + getPreviousResult: sinon.spy(), + getRequestResult: sinon.spy(), + getSubRequests: sinon.spy(), + invalidateOnBuild: sinon.spy(), + invalidateOnConfigKeyChange: sinon.spy(), + invalidateOnEnvChange: sinon.spy(), + invalidateOnFileDelete: sinon.spy(), + invalidateOnFileUpdate: sinon.spy(), + invalidateOnOptionChange: sinon.spy(), + invalidateOnStartup: sinon.spy(), + runRequest: sinon.spy(request => { + return request.run({ + api: mockRunApi, + options, + }); + }), + }; + return mockRunApi; }; ['rust', 'js'].forEach(backend => { @@ -44,6 +50,7 @@ describe('ConfigRequest tests', () => { }); it('can execute a config request', async () => { + const mockRunApi = getMockRunApi(); await runConfigRequest(mockRunApi, { id: 'config_request_test', invalidateOnBuild: false, @@ -56,7 +63,39 @@ describe('ConfigRequest tests', () => { }); }); + if (backend === 'rust') { + // Adding this here mostly to prove that the rust backend is actually running on + // this suite + it('errors out if the options are missing projectRoot', async () => { + const mockRunApi = getMockRunApi({}); + let error: Error | null = null; + try { + await runConfigRequest(mockRunApi, { + id: 'config_request_test', + invalidateOnBuild: false, + invalidateOnConfigKeyChange: [], + invalidateOnFileCreate: [], + invalidateOnEnvChange: new Set(), + invalidateOnOptionChange: new Set(), + invalidateOnStartup: false, + invalidateOnFileChange: new Set([ + toProjectPath(projectRoot, 'path1'), + toProjectPath(projectRoot, 'path2'), + ]), + }); + } catch (e) { + error = e; + } + + assert.equal( + error?.message, + '[napi] Missing required projectRoot options field', + ); + }); + } + it('forwards "invalidateOnFileChange" calls to runAPI', async () => { + const mockRunApi = getMockRunApi(); await runConfigRequest(mockRunApi, { id: 'config_request_test', invalidateOnBuild: false, From e510d51e8445e2a07b0f512815b9beb97aa2b5a0 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Fri, 19 Apr 2024 11:32:09 +1000 Subject: [PATCH 04/26] Fix more config API bits --- .../src/core/requests/config_request/mod.rs | 37 +++--------- .../requests/request_api/js_request_api.rs | 20 +++++-- .../src/core/requests/request_api/mod.rs | 6 +- .../core/test/requests/ConfigRequest.test.js | 57 +++++++++++++++++++ 4 files changed, 87 insertions(+), 33 deletions(-) diff --git a/crates/node-bindings/src/core/requests/config_request/mod.rs b/crates/node-bindings/src/core/requests/config_request/mod.rs index b623a85685c..39ebd24b1a8 100644 --- a/crates/node-bindings/src/core/requests/config_request/mod.rs +++ b/crates/node-bindings/src/core/requests/config_request/mod.rs @@ -17,26 +17,15 @@ pub struct ConfigKeyChange { } #[napi(object)] -pub struct InternalFileInvalidation { - pub file_path: ProjectPath, -} - -#[napi(object)] -pub struct InternalGlobInvalidation { - pub glob: InternalGlob, -} - -#[napi(object)] -pub struct InternalFileAboveInvalidation { - pub file_name: String, - pub above_file_path: ProjectPath, -} - -#[napi(object)] +#[derive(Clone)] pub struct InternalFileCreateInvalidation { - pub file: Option, - pub glob: Option, - pub file_above: Option, + // file + pub file_path: Option, + // glob + pub glob: Option, + // file above + pub file_name: Option, + pub above_file_path: Option, } #[napi(object)] @@ -90,15 +79,7 @@ pub fn run_config_request( } for invalidation in &config_request.invalidate_on_file_create { - if let Some(file) = &invalidation.file { - api.invalidate_on_file_create(Path::new(&file.file_path))?; - } - if let Some(glob) = &invalidation.glob { - api.invalidate_on_file_create(Path::new(&glob.glob))?; - } - if let Some(file_above) = &invalidation.file_above { - api.invalidate_on_file_create(Path::new(&file_above.above_file_path))?; - } + api.invalidate_on_file_create(invalidation)?; } for env in &config_request.invalidate_on_env_change { diff --git a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs index 07beadfbc37..83b5035f15d 100644 --- a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs +++ b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs @@ -1,9 +1,10 @@ use std::path::Path; use std::rc::Rc; -use napi::{Env, JsObject}; +use napi::{Env, JsObject, JsUnknown}; use crate::core::requests::call_method; +use crate::core::requests::config_request::InternalFileCreateInvalidation; use crate::core::requests::request_api::{RequestApi, RequestApiResult}; pub struct JSRequestApi { @@ -41,13 +42,24 @@ impl RequestApi for JSRequestApi { Ok(()) } - fn invalidate_on_file_create(&self, path: &Path) -> RequestApiResult<()> { - let path_js_string = self.env.create_string(path.to_str().unwrap())?; + fn invalidate_on_file_create( + &self, + invalidation: &InternalFileCreateInvalidation, + ) -> RequestApiResult<()> { + use napi::bindgen_prelude::ToNapiValue; + use napi::NapiValue; + + let js_invalidation = unsafe { + JsUnknown::from_raw( + self.env.raw(), + ToNapiValue::to_napi_value(self.env.raw(), invalidation.clone())?, + ) + }?; call_method( &self.env, &self.js_object, "invalidateOnFileCreate", - &[&path_js_string.into_unknown()], + &[&js_invalidation], )?; Ok(()) } diff --git a/crates/node-bindings/src/core/requests/request_api/mod.rs b/crates/node-bindings/src/core/requests/request_api/mod.rs index df4e7d94ae3..9ad64eca8b0 100644 --- a/crates/node-bindings/src/core/requests/request_api/mod.rs +++ b/crates/node-bindings/src/core/requests/request_api/mod.rs @@ -1,6 +1,7 @@ use mockall::automock; use std::path::Path; +use crate::core::requests::config_request::InternalFileCreateInvalidation; use napi::Env; pub mod js_request_api; @@ -15,7 +16,10 @@ pub trait RequestApi { /// Invalidate the current request when a file at `path` is deleted fn invalidate_on_file_delete(&self, path: &Path) -> RequestApiResult<()>; /// Invalidate the current request when a file at `path` is created - fn invalidate_on_file_create(&self, path: &Path) -> RequestApiResult<()>; + fn invalidate_on_file_create( + &self, + path: &InternalFileCreateInvalidation, + ) -> RequestApiResult<()>; /// Invalidate the current request when a config key from the configuration /// file at path is changed fn invalidate_on_config_key_change( diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index f1b345417cd..67dd0853cad 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -118,6 +118,63 @@ describe('ConfigRequest tests', () => { mockCast(mockRunApi.invalidateOnFileUpdate).calledWith('path1'), 'Invalidate was called with path1', ); + assert( + mockCast(mockRunApi.invalidateOnFileUpdate).calledWith('path2'), + 'Invalidate was called with path2', + ); + assert( + mockCast(mockRunApi.invalidateOnFileDelete).calledWith('path1'), + 'Invalidate was called with path1', + ); + assert( + mockCast(mockRunApi.invalidateOnFileDelete).calledWith('path2'), + 'Invalidate was called with path2', + ); + }); + + it('forwards "invalidateOnFileCreate" calls to runAPI', async () => { + const mockRunApi = getMockRunApi(); + await runConfigRequest(mockRunApi, { + id: 'config_request_test', + invalidateOnBuild: false, + invalidateOnConfigKeyChange: [], + invalidateOnFileCreate: [ + {filePath: toProjectPath(projectRoot, 'filePath')}, + {glob: toProjectPath(projectRoot, 'glob')}, + { + fileName: 'package.json', + aboveFilePath: toProjectPath(projectRoot, 'fileAbove'), + }, + ], + invalidateOnEnvChange: new Set(), + invalidateOnOptionChange: new Set(), + invalidateOnStartup: false, + invalidateOnFileChange: new Set(), + }); + + assert( + mockCast(mockRunApi.invalidateOnFileCreate).called, + 'Invalidate was called', + ); + assert( + mockCast(mockRunApi.invalidateOnFileCreate).calledWithMatch({ + filePath: 'filePath', + }), + 'Invalidate was called for path', + ); + assert( + mockCast(mockRunApi.invalidateOnFileCreate).calledWithMatch({ + glob: 'glob', + }), + 'Invalidate was called for glob', + ); + assert( + mockCast(mockRunApi.invalidateOnFileCreate).calledWithMatch({ + fileName: 'package.json', + aboveFilePath: 'fileAbove', + }), + 'Invalidate was called for fileAbove', + ); }); }); }); From f455d03281134466f0bee1bfc8f445bf9ffa32d9 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Fri, 19 Apr 2024 14:13:49 +1000 Subject: [PATCH 05/26] Add missing tests and fix bug in config request --- .../core/core/src/requests/ConfigRequest.js | 1 + .../core/test/requests/ConfigRequest.test.js | 123 +++++++++++++----- 2 files changed, 90 insertions(+), 34 deletions(-) diff --git a/packages/core/core/src/requests/ConfigRequest.js b/packages/core/core/src/requests/ConfigRequest.js index 2ffa6c70035..a9e1384a8d0 100644 --- a/packages/core/core/src/requests/ConfigRequest.js +++ b/packages/core/core/src/requests/ConfigRequest.js @@ -163,6 +163,7 @@ export async function runConfigRequest( invalidateOnConfigKeyChange.length === 0 && invalidateOnFileCreate.length === 0 && invalidateOnOptionChange.size === 0 && + invalidateOnEnvChange.size === 0 && !invalidateOnStartup && !invalidateOnBuild ) { diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index 67dd0853cad..3c19d701801 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -7,6 +7,7 @@ import {runConfigRequest} from '../../src/requests/ConfigRequest'; import type {RunAPI} from '../../src/RequestTracker'; import sinon from 'sinon'; import {toProjectPath} from '../../src/projectPath'; +import type {ConfigRequest} from '../../src/requests/ConfigRequest'; // $FlowFixMe unclear-type forgive me const mockCast = (f: any): any => f; @@ -40,7 +41,18 @@ describe('ConfigRequest tests', () => { return mockRunApi; }; - ['rust', 'js'].forEach(backend => { + const baseRequest: ConfigRequest = { + id: 'config_request_test', + invalidateOnBuild: false, + invalidateOnConfigKeyChange: [], + invalidateOnFileCreate: [], + invalidateOnEnvChange: new Set(), + invalidateOnOptionChange: new Set(), + invalidateOnStartup: false, + invalidateOnFileChange: new Set(), + }; + + [('rust', 'js')].forEach(backend => { describe(`${backend} backed`, () => { beforeEach(() => { setFeatureFlags({ @@ -52,14 +64,7 @@ describe('ConfigRequest tests', () => { it('can execute a config request', async () => { const mockRunApi = getMockRunApi(); await runConfigRequest(mockRunApi, { - id: 'config_request_test', - invalidateOnBuild: false, - invalidateOnConfigKeyChange: [], - invalidateOnFileCreate: [], - invalidateOnEnvChange: new Set(), - invalidateOnOptionChange: new Set(), - invalidateOnStartup: false, - invalidateOnFileChange: new Set(), + ...baseRequest, }); }); @@ -71,17 +76,7 @@ describe('ConfigRequest tests', () => { let error: Error | null = null; try { await runConfigRequest(mockRunApi, { - id: 'config_request_test', - invalidateOnBuild: false, - invalidateOnConfigKeyChange: [], - invalidateOnFileCreate: [], - invalidateOnEnvChange: new Set(), - invalidateOnOptionChange: new Set(), - invalidateOnStartup: false, - invalidateOnFileChange: new Set([ - toProjectPath(projectRoot, 'path1'), - toProjectPath(projectRoot, 'path2'), - ]), + ...baseRequest, }); } catch (e) { error = e; @@ -97,13 +92,7 @@ describe('ConfigRequest tests', () => { it('forwards "invalidateOnFileChange" calls to runAPI', async () => { const mockRunApi = getMockRunApi(); await runConfigRequest(mockRunApi, { - id: 'config_request_test', - invalidateOnBuild: false, - invalidateOnConfigKeyChange: [], - invalidateOnFileCreate: [], - invalidateOnEnvChange: new Set(), - invalidateOnOptionChange: new Set(), - invalidateOnStartup: false, + ...baseRequest, invalidateOnFileChange: new Set([ toProjectPath(projectRoot, 'path1'), toProjectPath(projectRoot, 'path2'), @@ -135,9 +124,7 @@ describe('ConfigRequest tests', () => { it('forwards "invalidateOnFileCreate" calls to runAPI', async () => { const mockRunApi = getMockRunApi(); await runConfigRequest(mockRunApi, { - id: 'config_request_test', - invalidateOnBuild: false, - invalidateOnConfigKeyChange: [], + ...baseRequest, invalidateOnFileCreate: [ {filePath: toProjectPath(projectRoot, 'filePath')}, {glob: toProjectPath(projectRoot, 'glob')}, @@ -146,10 +133,6 @@ describe('ConfigRequest tests', () => { aboveFilePath: toProjectPath(projectRoot, 'fileAbove'), }, ], - invalidateOnEnvChange: new Set(), - invalidateOnOptionChange: new Set(), - invalidateOnStartup: false, - invalidateOnFileChange: new Set(), }); assert( @@ -176,6 +159,78 @@ describe('ConfigRequest tests', () => { 'Invalidate was called for fileAbove', ); }); + + it('forwards "invalidateOnEnvChange" calls to runAPI', async () => { + const mockRunApi = getMockRunApi(); + await runConfigRequest(mockRunApi, { + ...baseRequest, + invalidateOnEnvChange: new Set(['env1', 'env2']), + }); + + assert( + mockCast(mockRunApi.invalidateOnEnvChange).called, + 'Invalidate was called', + ); + assert( + mockCast(mockRunApi.invalidateOnEnvChange).calledWithMatch('env1'), + 'Invalidate was called for env1', + ); + assert( + mockCast(mockRunApi.invalidateOnEnvChange).calledWithMatch('env2'), + 'Invalidate was called for env1', + ); + }); + + it('forwards "invalidateOnOptionChange" calls to runAPI', async () => { + const mockRunApi = getMockRunApi(); + await runConfigRequest(mockRunApi, { + ...baseRequest, + invalidateOnOptionChange: new Set(['option1', 'option2']), + }); + + assert( + mockCast(mockRunApi.invalidateOnOptionChange).called, + 'Invalidate was called', + ); + assert( + mockCast(mockRunApi.invalidateOnOptionChange).calledWithMatch( + 'option1', + ), + 'Invalidate was called for option1', + ); + assert( + mockCast(mockRunApi.invalidateOnOptionChange).calledWithMatch( + 'option2', + ), + 'Invalidate was called for option2', + ); + }); + + it('forwards "invalidateOnStartup" calls to runAPI', async () => { + const mockRunApi = getMockRunApi(); + await runConfigRequest(mockRunApi, { + ...baseRequest, + invalidateOnStartup: true, + }); + + assert( + mockCast(mockRunApi.invalidateOnStartup).called, + 'Invalidate was called', + ); + }); + + it('forwards "invalidateOnBuild" calls to runAPI', async () => { + const mockRunApi = getMockRunApi(); + await runConfigRequest(mockRunApi, { + ...baseRequest, + invalidateOnBuild: true, + }); + + assert( + mockCast(mockRunApi.invalidateOnBuild).called, + 'Invalidate was called', + ); + }); }); }); }); From 7533648423686c6d3880f562fecdd196c0389d33 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Fri, 19 Apr 2024 14:17:19 +1000 Subject: [PATCH 06/26] Add missing documentation --- crates/node-bindings/src/core/requests/mod.rs | 6 ++++++ crates/node-bindings/src/core/requests/request_api/mod.rs | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/crates/node-bindings/src/core/requests/mod.rs b/crates/node-bindings/src/core/requests/mod.rs index bcea82b22a1..35275746675 100644 --- a/crates/node-bindings/src/core/requests/mod.rs +++ b/crates/node-bindings/src/core/requests/mod.rs @@ -64,6 +64,12 @@ pub fn call_method( Ok(()) } +/// JavaScript API for running a config request. +/// At the moment the request fields themselves will be copied on call. +/// +/// This is not efficient but can be worked around when it becomes an issue. +/// +/// This should have exhaustive unit-tests on `packages/core/core/test/requests/ConfigRequest.test.js`. #[napi] fn napi_run_config_request( env: Env, diff --git a/crates/node-bindings/src/core/requests/request_api/mod.rs b/crates/node-bindings/src/core/requests/request_api/mod.rs index 9ad64eca8b0..2542a24c01a 100644 --- a/crates/node-bindings/src/core/requests/request_api/mod.rs +++ b/crates/node-bindings/src/core/requests/request_api/mod.rs @@ -9,6 +9,13 @@ pub mod js_request_api; // TODO: Move this into an associated type of the struct pub type RequestApiResult = napi::Result; +/// RequestTracker API with the requests. +/// +/// We will implement these as we need them. While working on integrating +/// with the existing JavaScript codebase, `JSRequestApi` will be used and will +/// delegate these calls into the JavaScript implementation. +/// +/// `mockall::automock` also generates a `MockRequestApi` to be used internally. #[automock] pub trait RequestApi { /// Invalidate the current request when a file at `path` is updated @@ -37,6 +44,7 @@ pub trait RequestApi { /// Invalidate the current request on option changes fn invalidate_on_option_change(&self, option: &str) -> RequestApiResult<()>; + // Missing functions: // fn getInvalidations() -> Vec; // fn store_result(result: RequestResult, cacheKey: &str); // fn get_request_result(contentKey: &str); From d4da5046b185379471a337520c6db4bbe6cdf9f9 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Fri, 19 Apr 2024 14:46:32 +1000 Subject: [PATCH 07/26] Try implement read_config bits --- .../src/core/requests/config_request/mod.rs | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/crates/node-bindings/src/core/requests/config_request/mod.rs b/crates/node-bindings/src/core/requests/config_request/mod.rs index 39ebd24b1a8..067b0dedcbd 100644 --- a/crates/node-bindings/src/core/requests/config_request/mod.rs +++ b/crates/node-bindings/src/core/requests/config_request/mod.rs @@ -43,13 +43,50 @@ pub struct ConfigRequest { pub invalidate_on_build: bool, } +/// Read a TOML or JSON configuration file as a value and return it +fn read_config(input_fs: &impl FileSystem, config_path: &Path) -> napi::Result { + let contents = input_fs.read_to_string(config_path)?; + let Some(extension) = config_path.extension().map(|ext| ext.to_str()).flatten() else { + // TODO: current JS behaviour might be to read it as JSON + return Err(napi::Error::from_reason( + "Configuration file has no extension or extension isn't unicode", + )); + }; + let contents = match extension { + "json" => serde_json::from_str(&contents) + .map_err(|e| napi::Error::new(napi::Status::GenericFailure, e.to_string())), + "toml" => toml::from_str(&contents) + .map_err(|e| napi::Error::new(napi::Status::GenericFailure, e.to_string())), + extension => Err(napi::Error::from_reason(format!( + "Invalid configuration format: {}", + extension + ))), + }?; + + Ok(contents) +} + +fn hash_serde_value(value: &serde_json::Value) -> anyhow::Result { + // TODO: this doesn't handle sorting keys + Ok(crate::hash::hash_string(serde_json::to_string(value)?)) +} + fn get_config_key_content_hash( - file_path: &str, config_key: &str, input_fs: &impl FileSystem, project_root: &str, + file_path: &str, ) -> napi::Result { - todo!("") + let contents = read_config(input_fs, Path::new(file_path))?; + + let Some(config_value) = contents.get(config_key) else { + // TODO: need to try to match behaviour of `ConfigRequest.js` + return Ok("".to_string()); + }; + + let content_hash = + hash_serde_value(config_value).map_err(|err| napi::Error::from_reason(err.to_string()))?; + Ok(content_hash) } pub fn run_config_request( @@ -66,10 +103,10 @@ pub fn run_config_request( for config_key_change in &config_request.invalidate_on_config_key_change { let content_hash = get_config_key_content_hash( - &config_key_change.file_path, &config_key_change.config_key, input_fs, &project_root, + &config_key_change.file_path, )?; api.invalidate_on_config_key_change( Path::new(&config_key_change.file_path), From 778e6949eda24f32ea6e9a40ffe8e7058045ca6d Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Mon, 22 Apr 2024 11:57:46 +1000 Subject: [PATCH 08/26] Fix flow errors --- packages/core/core/test/test-utils.js | 1 + packages/core/integration-tests/test/cache.js | 3 +++ packages/core/rust/index.js.flow | 15 +++++++++++++++ 3 files changed, 19 insertions(+) diff --git a/packages/core/core/test/test-utils.js b/packages/core/core/test/test-utils.js index b44e22be99f..4e4bc66e534 100644 --- a/packages/core/core/test/test-utils.js +++ b/packages/core/core/test/test-utils.js @@ -54,6 +54,7 @@ export const DEFAULT_OPTIONS: ParcelOptions = { featureFlags: { exampleFeature: false, configKeyInvalidation: false, + parcelV3: false, }, }; diff --git a/packages/core/integration-tests/test/cache.js b/packages/core/integration-tests/test/cache.js index f6a8bf07180..5abdf4ec6cb 100644 --- a/packages/core/integration-tests/test/cache.js +++ b/packages/core/integration-tests/test/cache.js @@ -1319,6 +1319,7 @@ describe('cache', function () { featureFlags: { exampleFeature: false, configKeyInvalidation: true, + parcelV3: false, }, async setup() { let pkgFile = path.join(inputDir, 'package.json'); @@ -1379,6 +1380,7 @@ describe('cache', function () { featureFlags: { exampleFeature: false, configKeyInvalidation: true, + parcelV3: false, }, async setup() { let pkgFile = path.join(inputDir, 'package.json'); @@ -1439,6 +1441,7 @@ describe('cache', function () { featureFlags: { exampleFeature: false, configKeyInvalidation: true, + parcelV3: false, }, async setup() { let pkgFile = path.join(inputDir, 'package.json'); diff --git a/packages/core/rust/index.js.flow b/packages/core/rust/index.js.flow index a8516acf705..0c406a34715 100644 --- a/packages/core/rust/index.js.flow +++ b/packages/core/rust/index.js.flow @@ -3,6 +3,21 @@ import type {FileCreateInvalidation} from '@parcel/types'; declare export var init: void | (() => void); +export type ProjectPath = any; +export interface ConfigRequest { + id: string, + invalidateOnFileChange: Array, + invalidateOnConfigKeyChange: Array, + invalidateOnFileCreate: Array, + invalidateOnEnvChange: Array, + invalidateOnOptionChange: Array, + invalidateOnStartup: boolean, + invalidateOnBuild: boolean, +} +export interface RequestOptions { +} + +declare export function napiRunConfigRequest(configRequest: ConfigRequest, api: any, options: any): void declare export function findAncestorFile(filenames: Array, from: string, root: string): string | null declare export function findFirstFile(names: Array): string | null declare export function findNodeModule(module: string, from: string): string | null From d0445920cb805c96f1bfc6ca94ed745bbb4eb8a6 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Mon, 22 Apr 2024 13:56:58 +1000 Subject: [PATCH 09/26] Add more tests for config request implementation --- crates/node-bindings/src/core/mod.rs | 3 + .../src/core/requests/config_request/mod.rs | 219 +++++++++++++++++- crates/node-bindings/src/core/requests/mod.rs | 5 +- .../src/core/requests/request_api/mod.rs | 1 - .../node-bindings/src/core/test_utils/mod.rs | 185 +++++++++++++++ 5 files changed, 405 insertions(+), 8 deletions(-) create mode 100644 crates/node-bindings/src/core/test_utils/mod.rs diff --git a/crates/node-bindings/src/core/mod.rs b/crates/node-bindings/src/core/mod.rs index 9a926cd401d..6dfe90309c2 100644 --- a/crates/node-bindings/src/core/mod.rs +++ b/crates/node-bindings/src/core/mod.rs @@ -2,3 +2,6 @@ /// Request types and run functions mod requests; + +#[cfg(test)] +pub(self) mod test_utils; diff --git a/crates/node-bindings/src/core/requests/config_request/mod.rs b/crates/node-bindings/src/core/requests/config_request/mod.rs index 067b0dedcbd..9c2677fd6dc 100644 --- a/crates/node-bindings/src/core/requests/config_request/mod.rs +++ b/crates/node-bindings/src/core/requests/config_request/mod.rs @@ -1,3 +1,7 @@ +//! Implements the `ConfigRequest` execution in rust. +//! +//! This is a rewrite of the `packages/core/core/src/requests/ConfigRequest.js` +//! file. use std::path::Path; use napi_derive::napi; @@ -17,7 +21,7 @@ pub struct ConfigKeyChange { } #[napi(object)] -#[derive(Clone)] +#[derive(Clone, PartialEq)] pub struct InternalFileCreateInvalidation { // file pub file_path: Option, @@ -66,15 +70,19 @@ fn read_config(input_fs: &impl FileSystem, config_path: &Path) -> napi::Result anyhow::Result { // TODO: this doesn't handle sorting keys Ok(crate::hash::hash_string(serde_json::to_string(value)?)) } +/// Hash a certain key in a configuration file. fn get_config_key_content_hash( config_key: &str, input_fs: &impl FileSystem, - project_root: &str, + // TODO: This be used to convert the file_path to a project_root relative path + _project_root: &str, file_path: &str, ) -> napi::Result { let contents = read_config(input_fs, Path::new(file_path))?; @@ -89,6 +97,9 @@ fn get_config_key_content_hash( Ok(content_hash) } +/// A config request triggers several invalidations to be tracked. +/// +/// This is ported to rust to serve as an example of Parcel requests being ported. pub fn run_config_request( config_request: &ConfigRequest, api: &impl RequestApi, @@ -147,11 +158,12 @@ mod test { use crate::core::requests::config_request::run_config_request; use crate::core::requests::request_api::MockRequestApi; + use crate::core::test_utils::InMemoryFileSystem; use super::*; #[test] - fn test_run_config_request() { + fn test_run_empty_config_request_does_nothing() { let config_request = ConfigRequest { id: "".to_string(), invalidate_on_file_change: vec![], @@ -162,10 +174,209 @@ mod test { invalidate_on_startup: false, invalidate_on_build: false, }; + // The mock will panic if it's called with no mock set let request_api = MockRequestApi::new(); let file_system = OsFileSystem::default(); let project_root = ""; - run_config_request(&config_request, &request_api, &file_system, project_root).unwrap() + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap(); + } + + #[test] + fn test_run_config_request_with_invalidate_on_file_change() { + let config_request = ConfigRequest { + id: "".to_string(), + invalidate_on_file_change: vec!["path1".to_string(), "path2".to_string()], + invalidate_on_config_key_change: vec![], + invalidate_on_file_create: vec![], + invalidate_on_env_change: vec![], + invalidate_on_option_change: vec![], + invalidate_on_startup: false, + invalidate_on_build: false, + }; + // The mock will panic if it's called with no mock set + let mut request_api = MockRequestApi::new(); + let file_system = OsFileSystem::default(); + let project_root = ""; + + request_api + .expect_invalidate_on_file_update() + .times(2) + .withf(|p| p.to_str().unwrap() == "path1" || p.to_str().unwrap() == "path2") + .returning(|_| Ok(())); + request_api + .expect_invalidate_on_file_delete() + .times(2) + .withf(|p| p.to_str().unwrap() == "path1" || p.to_str().unwrap() == "path2") + .returning(|_| Ok(())); + + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap(); + } + + #[test] + fn test_run_config_request_with_invalidate_on_file_create() { + let config_request = ConfigRequest { + id: "".to_string(), + invalidate_on_file_change: vec![], + invalidate_on_config_key_change: vec![], + invalidate_on_file_create: vec![InternalFileCreateInvalidation { + file_path: Some("path1".to_string()), + glob: None, + file_name: None, + above_file_path: None, + }], + invalidate_on_env_change: vec![], + invalidate_on_option_change: vec![], + invalidate_on_startup: false, + invalidate_on_build: false, + }; + // The mock will panic if it's called with no mock set + let mut request_api = MockRequestApi::new(); + let file_system = OsFileSystem::default(); + let project_root = ""; + + request_api + .expect_invalidate_on_file_create() + .times(1) + .withf(|p| { + *p == InternalFileCreateInvalidation { + file_path: Some("path1".to_string()), + glob: None, + file_name: None, + above_file_path: None, + } + }) + .returning(|_| Ok(())); + + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap(); + } + + #[test] + fn test_run_config_request_with_invalidate_on_env_change() { + let config_request = ConfigRequest { + id: "".to_string(), + invalidate_on_file_change: vec![], + invalidate_on_config_key_change: vec![], + invalidate_on_file_create: vec![], + invalidate_on_env_change: vec!["env1".to_string()], + invalidate_on_option_change: vec![], + invalidate_on_startup: false, + invalidate_on_build: false, + }; + // The mock will panic if it's called with no mock set + let mut request_api = MockRequestApi::new(); + let file_system = OsFileSystem::default(); + let project_root = ""; + + request_api + .expect_invalidate_on_env_change() + .times(1) + .withf(|p| p == "env1") + .returning(|_| Ok(())); + + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap(); + } + + #[test] + fn test_run_config_request_with_invalidate_on_option_change() { + let config_request = ConfigRequest { + id: "".to_string(), + invalidate_on_file_change: vec![], + invalidate_on_config_key_change: vec![], + invalidate_on_file_create: vec![], + invalidate_on_env_change: vec![], + invalidate_on_option_change: vec!["option1".to_string()], + invalidate_on_startup: false, + invalidate_on_build: false, + }; + // The mock will panic if it's called with no mock set + let mut request_api = MockRequestApi::new(); + let file_system = OsFileSystem::default(); + let project_root = ""; + + request_api + .expect_invalidate_on_option_change() + .times(1) + .withf(|p| p == "option1") + .returning(|_| Ok(())); + + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap(); + } + + #[test] + fn test_run_config_request_with_invalidate_on_startup() { + let config_request = ConfigRequest { + id: "".to_string(), + invalidate_on_file_change: vec![], + invalidate_on_config_key_change: vec![], + invalidate_on_file_create: vec![], + invalidate_on_env_change: vec![], + invalidate_on_option_change: vec![], + invalidate_on_startup: true, + invalidate_on_build: false, + }; + // The mock will panic if it's called with no mock set + let mut request_api = MockRequestApi::new(); + let file_system = OsFileSystem::default(); + let project_root = ""; + + request_api + .expect_invalidate_on_startup() + .times(1) + .returning(|| Ok(())); + + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap(); + } + + #[test] + fn test_run_config_request_with_invalidate_on_build() { + let config_request = ConfigRequest { + id: "".to_string(), + invalidate_on_file_change: vec![], + invalidate_on_config_key_change: vec![], + invalidate_on_file_create: vec![], + invalidate_on_env_change: vec![], + invalidate_on_option_change: vec![], + invalidate_on_startup: false, + invalidate_on_build: true, + }; + // The mock will panic if it's called with no mock set + let mut request_api = MockRequestApi::new(); + let file_system = OsFileSystem::default(); + let project_root = ""; + + request_api + .expect_invalidate_on_build() + .times(1) + .returning(|| Ok(())); + + run_config_request(&config_request, &request_api, &file_system, project_root).unwrap(); + } + + #[test] + fn test_read_json_config() { + let mut file_system = InMemoryFileSystem::default(); + let config_path = Path::new("/config.json"); + file_system.write_file(config_path, String::from(r#"{"key": "value"}"#)); + + let contents = read_config(&file_system, config_path).unwrap(); + assert_eq!(contents, serde_json::json!({"key": "value"})); + } + + #[test] + fn test_read_toml_config() { + let mut file_system = InMemoryFileSystem::default(); + let config_path = Path::new("/config.toml"); + file_system.write_file(config_path, String::from(r#"key = "value""#)); + + let contents = read_config(&file_system, config_path).unwrap(); + assert_eq!(contents, serde_json::json!({"key": "value"})); + } + + #[test] + fn test_hash_serde_value() { + let value = serde_json::json!({"key": "value", "key2": "value2"}); + let hash = hash_serde_value(&value).unwrap(); + assert_eq!(hash, "17666ca1af93de5d".to_string()); } } diff --git a/crates/node-bindings/src/core/requests/mod.rs b/crates/node-bindings/src/core/requests/mod.rs index 35275746675..6edb8b5b568 100644 --- a/crates/node-bindings/src/core/requests/mod.rs +++ b/crates/node-bindings/src/core/requests/mod.rs @@ -1,11 +1,10 @@ use std::rc::Rc; -use napi::bindgen_prelude::{FromNapiValue, ToNapiValue}; +use napi::bindgen_prelude::FromNapiValue; use napi::{Env, JsFunction, JsObject, JsString, JsUnknown, NapiRaw}; use napi_derive::napi; -use parcel_resolver::{FileSystem, OsFileSystem}; -use request_api::RequestApi; +use parcel_resolver::OsFileSystem; use crate::core::requests::config_request::ConfigRequest; use crate::core::requests::request_api::js_request_api::JSRequestApi; diff --git a/crates/node-bindings/src/core/requests/request_api/mod.rs b/crates/node-bindings/src/core/requests/request_api/mod.rs index 2542a24c01a..74679fac6db 100644 --- a/crates/node-bindings/src/core/requests/request_api/mod.rs +++ b/crates/node-bindings/src/core/requests/request_api/mod.rs @@ -2,7 +2,6 @@ use mockall::automock; use std::path::Path; use crate::core::requests::config_request::InternalFileCreateInvalidation; -use napi::Env; pub mod js_request_api; diff --git a/crates/node-bindings/src/core/test_utils/mod.rs b/crates/node-bindings/src/core/test_utils/mod.rs new file mode 100644 index 00000000000..2ec3249e398 --- /dev/null +++ b/crates/node-bindings/src/core/test_utils/mod.rs @@ -0,0 +1,185 @@ +//! Utilities for testing parcel core + +use parcel_resolver::FileSystem; +use std::collections::HashMap; +use std::path::{Component, Path, PathBuf}; + +/// In memory implementation of a file-system entry +enum InMemoryFileSystemEntry { + File { contents: String }, + Directory, +} + +/// In memory implementation of the `FileSystem` trait, for testing purpouses. +pub struct InMemoryFileSystem { + files: HashMap, + current_working_directory: PathBuf, +} + +impl InMemoryFileSystem { + /// Change the current working directory. Used for resolving relative paths. + pub fn set_current_working_directory(&mut self, cwd: PathBuf) { + self.current_working_directory = cwd; + } + + /// Create a directory at path. + pub fn create_directory(&mut self, path: impl AsRef) { + self + .files + .insert(path.as_ref().into(), InMemoryFileSystemEntry::Directory); + } + + /// Write a file at path. + pub fn write_file(&mut self, path: impl AsRef, contents: String) { + self.files.insert( + path.as_ref().into(), + InMemoryFileSystemEntry::File { contents }, + ); + } +} + +impl Default for InMemoryFileSystem { + fn default() -> Self { + Self { + files: Default::default(), + current_working_directory: PathBuf::from("/"), + } + } +} + +impl FileSystem for InMemoryFileSystem { + fn canonicalize>( + &self, + path: P, + _cache: &dashmap::DashMap>, + ) -> std::io::Result { + let path = path.as_ref(); + + let mut result = if path.is_absolute() { + vec![] + } else { + self.current_working_directory.components().collect() + }; + + let components = path.components(); + for component in components { + match component { + Component::Prefix(prefix) => { + result = vec![Component::Prefix(prefix)]; + } + Component::RootDir => { + result = vec![Component::RootDir]; + } + Component::CurDir => {} + Component::ParentDir => { + result.pop(); + } + Component::Normal(path) => { + result.push(Component::Normal(path)); + } + } + } + + Ok(PathBuf::from_iter(result)) + } + + fn read_to_string>(&self, path: P) -> std::io::Result { + self.files.get(path.as_ref()).map_or_else( + || { + Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + "file not found", + )) + }, + |entry| match entry { + InMemoryFileSystemEntry::File { contents } => Ok(contents.clone()), + InMemoryFileSystemEntry::Directory => Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "path is a directory", + )), + }, + ) + } + + fn is_file>(&self, path: P) -> bool { + let file = self.files.get(path.as_ref()); + matches!(file, Some(InMemoryFileSystemEntry::File { .. })) + } + + fn is_dir>(&self, path: P) -> bool { + let file = self.files.get(path.as_ref()); + matches!(file, Some(InMemoryFileSystemEntry::Directory { .. })) + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_canonicalize_noop() { + let fs = InMemoryFileSystem::default(); + let path = Path::new("/foo/bar"); + let result = fs.canonicalize(path, &Default::default()).unwrap(); + assert_eq!(result, path); + } + + #[test] + fn test_remove_relative_dots() { + let fs = InMemoryFileSystem::default(); + let result = fs + .canonicalize(Path::new("/foo/./bar"), &Default::default()) + .unwrap(); + assert_eq!(result, PathBuf::from("/foo/bar")); + } + + #[test] + fn test_remove_relative_parent_dots() { + let fs = InMemoryFileSystem::default(); + let result = fs + .canonicalize(Path::new("/foo/./bar/../baz/"), &Default::default()) + .unwrap(); + assert_eq!(result, PathBuf::from("/foo/baz")); + } + + #[test] + fn test_with_cwd() { + let mut fs = InMemoryFileSystem::default(); + fs.set_current_working_directory(PathBuf::from("/other")); + let result = fs + .canonicalize(Path::new("./foo/./bar/../baz/"), &Default::default()) + .unwrap(); + assert_eq!(result, PathBuf::from("/other/foo/baz")); + } + + #[test] + fn test_read_file() { + let mut fs = InMemoryFileSystem::default(); + fs.write_file(PathBuf::from("/foo/bar"), "contents".to_string()); + let result = fs.read_to_string(Path::new("/foo/bar")).unwrap(); + assert_eq!(result, "contents"); + } + + #[test] + fn test_read_file_not_found() { + let fs = InMemoryFileSystem::default(); + let result = fs.read_to_string(Path::new("/foo/bar")); + assert!(result.is_err()); + } + + #[test] + fn test_is_file() { + let mut fs = InMemoryFileSystem::default(); + fs.write_file(PathBuf::from("/foo/bar"), "contents".to_string()); + assert!(fs.is_file(Path::new("/foo/bar"))); + assert!(!fs.is_file(Path::new("/foo"))); + } + + #[test] + fn test_is_dir() { + let mut fs = InMemoryFileSystem::default(); + fs.create_directory(PathBuf::from("/foo")); + assert!(fs.is_dir(Path::new("/foo"))); + assert!(!fs.is_dir(Path::new("/foo/bar"))); + } +} From 274b3b468296fd06c4f33e9f22ab5dd5d303e29e Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Mon, 22 Apr 2024 14:46:34 +1000 Subject: [PATCH 10/26] Finish migrating config request --- .../node-bindings/src/core/filesystem/mod.rs | 106 ++++++++++++++++++ .../core/{ => filesystem}/test_utils/mod.rs | 2 +- crates/node-bindings/src/core/mod.rs | 4 +- .../src/core/requests/config_request/mod.rs | 10 +- crates/node-bindings/src/core/requests/mod.rs | 30 +++-- .../core/test/requests/ConfigRequest.test.js | 96 ++++++++++++++-- packages/utils/node-resolver-rs/src/fs.rs | 2 +- 7 files changed, 224 insertions(+), 26 deletions(-) create mode 100644 crates/node-bindings/src/core/filesystem/mod.rs rename crates/node-bindings/src/core/{ => filesystem}/test_utils/mod.rs (99%) diff --git a/crates/node-bindings/src/core/filesystem/mod.rs b/crates/node-bindings/src/core/filesystem/mod.rs new file mode 100644 index 00000000000..a24a8873509 --- /dev/null +++ b/crates/node-bindings/src/core/filesystem/mod.rs @@ -0,0 +1,106 @@ +use crate::core::requests::call_method; +use dashmap::DashMap; +use napi::bindgen_prelude::FromNapiValue; +use napi::{Env, JsObject}; +use parcel_resolver::FileSystem; +use std::path::{Path, PathBuf}; +use std::rc::Rc; + +#[cfg(test)] +pub(crate) mod test_utils; + +/// An implementation of `FileSystem` that delegates calls to a `JsObject`. +/// +/// This is going to be very slow at runtime due to the overhead of converting +/// between rust and JS types. +pub struct JSDelegateFileSystem { + env: Rc, + js_delegate: JsObject, +} + +impl JSDelegateFileSystem { + pub fn new(env: Rc, js_delegate: JsObject) -> Self { + Self { env, js_delegate } + } +} + +// Convert arbitrary errors to io errors. This is wrong; the `FileSystem` trait should use +// `anyhow::Result` +fn run_with_errors(block: impl FnOnce() -> anyhow::Result) -> Result { + let result = block(); + result.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err.to_string())) +} + +impl FileSystem for JSDelegateFileSystem { + fn canonicalize>( + &self, + path: P, + _cache: &DashMap>, + ) -> std::io::Result { + run_with_errors(|| { + let path = path.as_ref().to_str().unwrap(); + let js_path = self.env.create_string(path)?; + let result = call_method( + &self.env, + &self.js_delegate, + "canonicalize", + &[&js_path.into_unknown()], + )?; + let result_string = result.coerce_to_string()?; + let result_string = result_string.into_utf8()?.as_str()?.to_string(); + Ok(PathBuf::from(result_string)) + }) + } + + fn read_to_string>(&self, path: P) -> std::io::Result { + run_with_errors(|| { + let path = path.as_ref().to_str().unwrap(); + let js_path = self.env.create_string(path)?; + let result = call_method( + &self.env, + &self.js_delegate, + "readFileSync", + &[&js_path.into_unknown()], + )?; + let buffer = napi::JsBuffer::from_unknown(result)?; + let buffer = buffer.into_value()?; + let buffer: &[u8] = buffer.as_ref(); + let result = String::from_utf8(buffer.to_vec())?; + Ok(result) + }) + } + + fn is_file>(&self, path: P) -> bool { + run_with_errors(|| { + let path = path.as_ref().to_str().unwrap(); + let js_path = self.env.create_string(path)?; + let result = call_method( + &self.env, + &self.js_delegate, + "isFile", + &[&js_path.into_unknown()], + )?; + let result_bool = result.coerce_to_bool()?.get_value()?; + Ok(result_bool) + // TODO error handling is messed up here; this should return `Result + }) + .unwrap_or(false) + } + + fn is_dir>(&self, path: P) -> bool { + run_with_errors(|| { + let path = path.as_ref().to_str().unwrap(); + let js_path = self.env.create_string(path)?; + let result = call_method( + &self.env, + &self.js_delegate, + "isDir", + &[&js_path.into_unknown()], + )?; + let result_bool = result.coerce_to_bool()?.get_value()?; + Ok(result_bool) + // TODO error handling is messed up here; this should return `Result + }) + .unwrap_or(false) + } +} diff --git a/crates/node-bindings/src/core/test_utils/mod.rs b/crates/node-bindings/src/core/filesystem/test_utils/mod.rs similarity index 99% rename from crates/node-bindings/src/core/test_utils/mod.rs rename to crates/node-bindings/src/core/filesystem/test_utils/mod.rs index 2ec3249e398..fa92fbc81e4 100644 --- a/crates/node-bindings/src/core/test_utils/mod.rs +++ b/crates/node-bindings/src/core/filesystem/test_utils/mod.rs @@ -1,4 +1,4 @@ -//! Utilities for testing parcel core +//! Utilities for testing parcel core filesystem use parcel_resolver::FileSystem; use std::collections::HashMap; diff --git a/crates/node-bindings/src/core/mod.rs b/crates/node-bindings/src/core/mod.rs index 6dfe90309c2..7abd3086ed8 100644 --- a/crates/node-bindings/src/core/mod.rs +++ b/crates/node-bindings/src/core/mod.rs @@ -1,7 +1,5 @@ //! Core re-implementation in Rust +mod filesystem; /// Request types and run functions mod requests; - -#[cfg(test)] -pub(self) mod test_utils; diff --git a/crates/node-bindings/src/core/requests/config_request/mod.rs b/crates/node-bindings/src/core/requests/config_request/mod.rs index 9c2677fd6dc..da67d84dc1b 100644 --- a/crates/node-bindings/src/core/requests/config_request/mod.rs +++ b/crates/node-bindings/src/core/requests/config_request/mod.rs @@ -81,11 +81,13 @@ fn hash_serde_value(value: &serde_json::Value) -> anyhow::Result { fn get_config_key_content_hash( config_key: &str, input_fs: &impl FileSystem, - // TODO: This be used to convert the file_path to a project_root relative path - _project_root: &str, + project_root: &str, file_path: &str, ) -> napi::Result { - let contents = read_config(input_fs, Path::new(file_path))?; + let mut path = Path::new(project_root).to_path_buf(); + path.push(file_path); + + let contents = read_config(input_fs, &path)?; let Some(config_value) = contents.get(config_key) else { // TODO: need to try to match behaviour of `ConfigRequest.js` @@ -156,9 +158,9 @@ struct RequestOptions {} mod test { use parcel_resolver::OsFileSystem; + use crate::core::filesystem::test_utils::InMemoryFileSystem; use crate::core::requests::config_request::run_config_request; use crate::core::requests::request_api::MockRequestApi; - use crate::core::test_utils::InMemoryFileSystem; use super::*; diff --git a/crates/node-bindings/src/core/requests/mod.rs b/crates/node-bindings/src/core/requests/mod.rs index 6edb8b5b568..45474c6d9c0 100644 --- a/crates/node-bindings/src/core/requests/mod.rs +++ b/crates/node-bindings/src/core/requests/mod.rs @@ -4,8 +4,7 @@ use napi::bindgen_prelude::FromNapiValue; use napi::{Env, JsFunction, JsObject, JsString, JsUnknown, NapiRaw}; use napi_derive::napi; -use parcel_resolver::OsFileSystem; - +use crate::core::filesystem::JSDelegateFileSystem; use crate::core::requests::config_request::ConfigRequest; use crate::core::requests::request_api::js_request_api::JSRequestApi; @@ -19,12 +18,18 @@ mod request_api; /// Uses raw NAPI casts, but checks that object field is a function pub fn get_function(env: &Env, js_object: &JsObject, field_name: &str) -> napi::Result { let Some(method): Option = js_object.get(field_name)? else { - return Err(napi::Error::from_reason("[napi] Method not found")); + return Err(napi::Error::from_reason(format!( + "[napi] Method not found: {}", + field_name + ))); }; let function_class: JsUnknown = env.get_global()?.get_named_property("Function")?; let is_function = method.instanceof(function_class)?; if !is_function { - return Err(napi::Error::from_reason("[napi] Method is not a function")); + return Err(napi::Error::from_reason(format!( + "[napi] Method is not a function: {}", + field_name + ))); } let method_fn = unsafe { JsFunction::from_napi_value(env.raw(), method.raw()) }?; @@ -57,10 +62,10 @@ pub fn call_method( js_object: &JsObject, field_name: &str, args: &[&JsUnknown], -) -> napi::Result<()> { +) -> napi::Result { let method_fn = get_function(env, js_object, field_name)?; - method_fn.call(Some(&js_object), &args)?; - Ok(()) + let result = method_fn.call(Some(&js_object), &args)?; + Ok(result) } /// JavaScript API for running a config request. @@ -79,8 +84,15 @@ fn napi_run_config_request( // Technically we could move `env` to JSRequestAPI but in order to // be able to use env on more places we rc it. let env = Rc::new(env); - let api = JSRequestApi::new(env, api); - let input_fs = OsFileSystem::default(); + let api = JSRequestApi::new(env.clone(), api); + let input_fs = options.get("inputFS")?; + let Some(input_fs) = input_fs.map(|input_fs| JSDelegateFileSystem::new(env, input_fs)) else { + // We need to make the `FileSystem` trait object-safe so we can use dynamic + // dispatch. + return Err(napi::Error::from_reason( + "[napi] Missing required inputFS options field", + )); + }; let Some(project_root): Option = options.get("projectRoot")? else { return Err(napi::Error::from_reason( "[napi] Missing required projectRoot options field", diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index 3c19d701801..66eb144dae8 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -8,13 +8,22 @@ import type {RunAPI} from '../../src/RequestTracker'; import sinon from 'sinon'; import {toProjectPath} from '../../src/projectPath'; import type {ConfigRequest} from '../../src/requests/ConfigRequest'; +import {MemoryFS} from '@parcel/fs'; +import {hashString} from '@parcel/rust'; // $FlowFixMe unclear-type forgive me const mockCast = (f: any): any => f; describe('ConfigRequest tests', () => { - const projectRoot = ''; - const getMockRunApi = (options: mixed = {projectRoot}): RunAPI => { + const projectRoot = '/project_root/'; + let fs = new MemoryFS(); + beforeEach(() => { + fs = new MemoryFS(); + }); + + const getMockRunApi = ( + options: mixed = {projectRoot, inputFS: fs}, + ): RunAPI => { const mockRunApi = { storeResult: sinon.spy(), canSkipSubrequest: sinon.spy(), @@ -52,7 +61,7 @@ describe('ConfigRequest tests', () => { invalidateOnFileChange: new Set(), }; - [('rust', 'js')].forEach(backend => { + ['rust', 'js'].forEach(backend => { describe(`${backend} backed`, () => { beforeEach(() => { setFeatureFlags({ @@ -71,22 +80,48 @@ describe('ConfigRequest tests', () => { if (backend === 'rust') { // Adding this here mostly to prove that the rust backend is actually running on // this suite - it('errors out if the options are missing projectRoot', async () => { - const mockRunApi = getMockRunApi({}); + async function assertThrows(block: () => Promise) { let error: Error | null = null; try { - await runConfigRequest(mockRunApi, { - ...baseRequest, - }); + await block(); } catch (e) { error = e; } + assert(error != null, 'Function finished without errors'); + return error; + } + it('errors out if the options are missing projectRoot', async () => { + const mockRunApi = getMockRunApi({ + inputFS: fs, + }); + const error = await assertThrows(async () => { + await runConfigRequest(mockRunApi, { + ...baseRequest, + invalidateOnStartup: true, + }); + }); assert.equal( error?.message, '[napi] Missing required projectRoot options field', ); }); + + it('errors out if the options are missing inputFS', async () => { + const mockRunApi = getMockRunApi({ + projectRoot, + }); + const error = await assertThrows(async () => { + await runConfigRequest(mockRunApi, { + ...baseRequest, + invalidateOnStartup: true, + }); + }); + assert.equal( + error?.message, + '[napi] Missing required inputFS options field', + ); + }); } it('forwards "invalidateOnFileChange" calls to runAPI', async () => { @@ -231,6 +266,51 @@ describe('ConfigRequest tests', () => { 'Invalidate was called', ); }); + + it('forwards "invalidateOnConfigKeyChange" calls to runAPI', async () => { + await fs.mkdirp('/project_root'); + await fs.writeFile( + '/project_root/config.json', + JSON.stringify({key1: 'value1'}), + ); + sinon.spy(fs, 'readFile'); + sinon.spy(fs, 'readFileSync'); + const mockRunApi = getMockRunApi(); + await runConfigRequest(mockRunApi, { + ...baseRequest, + invalidateOnConfigKeyChange: [ + { + configKey: 'key1', + filePath: toProjectPath(projectRoot, '/project_root/config.json'), + }, + ], + }); + + if (backend === 'rust') { + const fsCall = mockCast(fs.readFileSync).getCall(0); + assert.deepEqual( + fsCall?.args, + ['/project_root/config.json'], + 'readFile was called', + ); + } else { + const fsCall = mockCast(fs.readFile).getCall(0); + assert.deepEqual( + fsCall?.args, + ['/project_root/config.json', 'utf8'], + 'readFile was called', + ); + } + + const call = mockCast(mockRunApi.invalidateOnConfigKeyChange).getCall( + 0, + ); + assert.deepEqual( + call.args, + ['config.json', 'key1', hashString('"value1"')], + 'Invalidate was called for key1', + ); + }); }); }); }); diff --git a/packages/utils/node-resolver-rs/src/fs.rs b/packages/utils/node-resolver-rs/src/fs.rs index 3ebae1502d3..f0685ad140e 100644 --- a/packages/utils/node-resolver-rs/src/fs.rs +++ b/packages/utils/node-resolver-rs/src/fs.rs @@ -7,7 +7,7 @@ use std::{ use crate::path::canonicalize; use dashmap::DashMap; -pub trait FileSystem: Send + Sync { +pub trait FileSystem { fn canonicalize>( &self, path: P, From d8a691ccaef15dc1bd884ff629afc178dfec9aff Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Mon, 22 Apr 2024 14:49:55 +1000 Subject: [PATCH 11/26] Fix flow errors on ConfigRequest.test.js --- .../core/test/requests/ConfigRequest.test.js | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index 66eb144dae8..d73b61aeacf 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -1,24 +1,25 @@ // @flow strict-local -import {setFeatureFlags, DEFAULT_FEATURE_FLAGS} from '@parcel/feature-flags'; +import WorkerFarm from '@parcel/workers'; import assert from 'assert'; - -import {runConfigRequest} from '../../src/requests/ConfigRequest'; -import type {RunAPI} from '../../src/RequestTracker'; import sinon from 'sinon'; -import {toProjectPath} from '../../src/projectPath'; -import type {ConfigRequest} from '../../src/requests/ConfigRequest'; +import {DEFAULT_FEATURE_FLAGS, setFeatureFlags} from '@parcel/feature-flags'; import {MemoryFS} from '@parcel/fs'; import {hashString} from '@parcel/rust'; +import type {ConfigRequest} from '../../src/requests/ConfigRequest'; +import type {RunAPI} from '../../src/RequestTracker'; +import {runConfigRequest} from '../../src/requests/ConfigRequest'; +import {toProjectPath} from '../../src/projectPath'; + // $FlowFixMe unclear-type forgive me const mockCast = (f: any): any => f; describe('ConfigRequest tests', () => { const projectRoot = '/project_root/'; - let fs = new MemoryFS(); + let fs = new MemoryFS(new WorkerFarm()); beforeEach(() => { - fs = new MemoryFS(); + fs = new MemoryFS(new WorkerFarm()); }); const getMockRunApi = ( @@ -287,14 +288,14 @@ describe('ConfigRequest tests', () => { }); if (backend === 'rust') { - const fsCall = mockCast(fs.readFileSync).getCall(0); + const fsCall = mockCast(fs).readFileSync.getCall(0); assert.deepEqual( fsCall?.args, ['/project_root/config.json'], 'readFile was called', ); } else { - const fsCall = mockCast(fs.readFile).getCall(0); + const fsCall = mockCast(fs).readFile.getCall(0); assert.deepEqual( fsCall?.args, ['/project_root/config.json', 'utf8'], From d455371b4262638dbab13bf59d9bd4b06d30ebfc Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Mon, 22 Apr 2024 14:51:07 +1000 Subject: [PATCH 12/26] Fix linter errors on added tests --- .../core/test/requests/ConfigRequest.test.js | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index d73b61aeacf..12b00c09170 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -15,6 +15,17 @@ import {toProjectPath} from '../../src/projectPath'; // $FlowFixMe unclear-type forgive me const mockCast = (f: any): any => f; +async function assertThrows(block: () => Promise) { + let error: Error | null = null; + try { + await block(); + } catch (e) { + error = e; + } + assert(error != null, 'Function finished without errors'); + return error; +} + describe('ConfigRequest tests', () => { const projectRoot = '/project_root/'; let fs = new MemoryFS(new WorkerFarm()); @@ -81,16 +92,6 @@ describe('ConfigRequest tests', () => { if (backend === 'rust') { // Adding this here mostly to prove that the rust backend is actually running on // this suite - async function assertThrows(block: () => Promise) { - let error: Error | null = null; - try { - await block(); - } catch (e) { - error = e; - } - assert(error != null, 'Function finished without errors'); - return error; - } it('errors out if the options are missing projectRoot', async () => { const mockRunApi = getMockRunApi({ From 3efd2322173364cbf3f878abc8adad5c02ac0ee6 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Mon, 22 Apr 2024 14:55:42 +1000 Subject: [PATCH 13/26] Minor improvements to rust code --- .../mod.rs | 2 - .../filesystem/js_delegate_file_system/mod.rs | 107 +++++++++++++++++ .../node-bindings/src/core/filesystem/mod.rs | 108 +----------------- .../src/core/requests/config_request/mod.rs | 2 +- crates/node-bindings/src/core/requests/mod.rs | 2 +- 5 files changed, 113 insertions(+), 108 deletions(-) rename crates/node-bindings/src/core/filesystem/{test_utils => in_memory_file_system}/mod.rs (99%) create mode 100644 crates/node-bindings/src/core/filesystem/js_delegate_file_system/mod.rs diff --git a/crates/node-bindings/src/core/filesystem/test_utils/mod.rs b/crates/node-bindings/src/core/filesystem/in_memory_file_system/mod.rs similarity index 99% rename from crates/node-bindings/src/core/filesystem/test_utils/mod.rs rename to crates/node-bindings/src/core/filesystem/in_memory_file_system/mod.rs index fa92fbc81e4..13353a2fbda 100644 --- a/crates/node-bindings/src/core/filesystem/test_utils/mod.rs +++ b/crates/node-bindings/src/core/filesystem/in_memory_file_system/mod.rs @@ -1,5 +1,3 @@ -//! Utilities for testing parcel core filesystem - use parcel_resolver::FileSystem; use std::collections::HashMap; use std::path::{Component, Path, PathBuf}; diff --git a/crates/node-bindings/src/core/filesystem/js_delegate_file_system/mod.rs b/crates/node-bindings/src/core/filesystem/js_delegate_file_system/mod.rs new file mode 100644 index 00000000000..6f9b82559a5 --- /dev/null +++ b/crates/node-bindings/src/core/filesystem/js_delegate_file_system/mod.rs @@ -0,0 +1,107 @@ +use std::path::{Path, PathBuf}; +use std::rc::Rc; + +use dashmap::DashMap; +use napi::bindgen_prelude::FromNapiValue; +use napi::{Env, JsObject}; + +use parcel_resolver::FileSystem; + +use crate::core::requests::call_method; + +/// An implementation of `FileSystem` that delegates calls to a `JsObject`. +/// +/// This is going to be very slow at runtime due to the overhead of converting +/// between rust and JS types. +pub struct JSDelegateFileSystem { + env: Rc, + js_delegate: JsObject, +} + +impl JSDelegateFileSystem { + pub fn new(env: Rc, js_delegate: JsObject) -> Self { + Self { env, js_delegate } + } +} + +// Convert arbitrary errors to io errors. This is wrong; the `FileSystem` trait should use +// `anyhow::Result` +fn run_with_errors(block: impl FnOnce() -> anyhow::Result) -> Result { + let result = block(); + result.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err.to_string())) +} + +impl FileSystem for JSDelegateFileSystem { + fn canonicalize>( + &self, + path: P, + _cache: &DashMap>, + ) -> std::io::Result { + run_with_errors(|| { + let path = path.as_ref().to_str().unwrap(); + let js_path = self.env.create_string(path)?; + let result = call_method( + &self.env, + &self.js_delegate, + "canonicalize", + &[&js_path.into_unknown()], + )?; + let result_string = result.coerce_to_string()?; + let result_string = result_string.into_utf8()?.as_str()?.to_string(); + Ok(PathBuf::from(result_string)) + }) + } + + fn read_to_string>(&self, path: P) -> std::io::Result { + run_with_errors(|| { + let path = path.as_ref().to_str().unwrap(); + let js_path = self.env.create_string(path)?; + let result = call_method( + &self.env, + &self.js_delegate, + "readFileSync", + &[&js_path.into_unknown()], + )?; + // Using buffer hopefully avoids a copy + let buffer = napi::JsBuffer::from_unknown(result)?; + let buffer = buffer.into_value()?; + let buffer: &[u8] = buffer.as_ref(); + let result = String::from_utf8(buffer.to_vec())?; + Ok(result) + }) + } + + fn is_file>(&self, path: P) -> bool { + run_with_errors(|| { + let path = path.as_ref().to_str().unwrap(); + let js_path = self.env.create_string(path)?; + let result = call_method( + &self.env, + &self.js_delegate, + "isFile", + &[&js_path.into_unknown()], + )?; + let result_bool = result.coerce_to_bool()?.get_value()?; + Ok(result_bool) + // TODO error handling is messed up here; this should return `Result + }) + .unwrap_or(false) + } + + fn is_dir>(&self, path: P) -> bool { + run_with_errors(|| { + let path = path.as_ref().to_str().unwrap(); + let js_path = self.env.create_string(path)?; + let result = call_method( + &self.env, + &self.js_delegate, + "isDir", + &[&js_path.into_unknown()], + )?; + let result_bool = result.coerce_to_bool()?.get_value()?; + Ok(result_bool) + // TODO error handling is messed up here; this should return `Result + }) + .unwrap_or(false) + } +} diff --git a/crates/node-bindings/src/core/filesystem/mod.rs b/crates/node-bindings/src/core/filesystem/mod.rs index a24a8873509..94686e69686 100644 --- a/crates/node-bindings/src/core/filesystem/mod.rs +++ b/crates/node-bindings/src/core/filesystem/mod.rs @@ -1,106 +1,6 @@ -use crate::core::requests::call_method; -use dashmap::DashMap; -use napi::bindgen_prelude::FromNapiValue; -use napi::{Env, JsObject}; -use parcel_resolver::FileSystem; -use std::path::{Path, PathBuf}; -use std::rc::Rc; +/// FileSystem implementation that delegates calls to a JS object +pub(crate) mod js_delegate_file_system; +/// In-memory file-system for testing #[cfg(test)] -pub(crate) mod test_utils; - -/// An implementation of `FileSystem` that delegates calls to a `JsObject`. -/// -/// This is going to be very slow at runtime due to the overhead of converting -/// between rust and JS types. -pub struct JSDelegateFileSystem { - env: Rc, - js_delegate: JsObject, -} - -impl JSDelegateFileSystem { - pub fn new(env: Rc, js_delegate: JsObject) -> Self { - Self { env, js_delegate } - } -} - -// Convert arbitrary errors to io errors. This is wrong; the `FileSystem` trait should use -// `anyhow::Result` -fn run_with_errors(block: impl FnOnce() -> anyhow::Result) -> Result { - let result = block(); - result.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err.to_string())) -} - -impl FileSystem for JSDelegateFileSystem { - fn canonicalize>( - &self, - path: P, - _cache: &DashMap>, - ) -> std::io::Result { - run_with_errors(|| { - let path = path.as_ref().to_str().unwrap(); - let js_path = self.env.create_string(path)?; - let result = call_method( - &self.env, - &self.js_delegate, - "canonicalize", - &[&js_path.into_unknown()], - )?; - let result_string = result.coerce_to_string()?; - let result_string = result_string.into_utf8()?.as_str()?.to_string(); - Ok(PathBuf::from(result_string)) - }) - } - - fn read_to_string>(&self, path: P) -> std::io::Result { - run_with_errors(|| { - let path = path.as_ref().to_str().unwrap(); - let js_path = self.env.create_string(path)?; - let result = call_method( - &self.env, - &self.js_delegate, - "readFileSync", - &[&js_path.into_unknown()], - )?; - let buffer = napi::JsBuffer::from_unknown(result)?; - let buffer = buffer.into_value()?; - let buffer: &[u8] = buffer.as_ref(); - let result = String::from_utf8(buffer.to_vec())?; - Ok(result) - }) - } - - fn is_file>(&self, path: P) -> bool { - run_with_errors(|| { - let path = path.as_ref().to_str().unwrap(); - let js_path = self.env.create_string(path)?; - let result = call_method( - &self.env, - &self.js_delegate, - "isFile", - &[&js_path.into_unknown()], - )?; - let result_bool = result.coerce_to_bool()?.get_value()?; - Ok(result_bool) - // TODO error handling is messed up here; this should return `Result - }) - .unwrap_or(false) - } - - fn is_dir>(&self, path: P) -> bool { - run_with_errors(|| { - let path = path.as_ref().to_str().unwrap(); - let js_path = self.env.create_string(path)?; - let result = call_method( - &self.env, - &self.js_delegate, - "isDir", - &[&js_path.into_unknown()], - )?; - let result_bool = result.coerce_to_bool()?.get_value()?; - Ok(result_bool) - // TODO error handling is messed up here; this should return `Result - }) - .unwrap_or(false) - } -} +pub(crate) mod in_memory_file_system; diff --git a/crates/node-bindings/src/core/requests/config_request/mod.rs b/crates/node-bindings/src/core/requests/config_request/mod.rs index da67d84dc1b..9948e76b35c 100644 --- a/crates/node-bindings/src/core/requests/config_request/mod.rs +++ b/crates/node-bindings/src/core/requests/config_request/mod.rs @@ -158,7 +158,7 @@ struct RequestOptions {} mod test { use parcel_resolver::OsFileSystem; - use crate::core::filesystem::test_utils::InMemoryFileSystem; + use crate::core::filesystem::in_memory_file_system::InMemoryFileSystem; use crate::core::requests::config_request::run_config_request; use crate::core::requests::request_api::MockRequestApi; diff --git a/crates/node-bindings/src/core/requests/mod.rs b/crates/node-bindings/src/core/requests/mod.rs index 45474c6d9c0..f81249f670b 100644 --- a/crates/node-bindings/src/core/requests/mod.rs +++ b/crates/node-bindings/src/core/requests/mod.rs @@ -4,7 +4,7 @@ use napi::bindgen_prelude::FromNapiValue; use napi::{Env, JsFunction, JsObject, JsString, JsUnknown, NapiRaw}; use napi_derive::napi; -use crate::core::filesystem::JSDelegateFileSystem; +use crate::core::filesystem::js_delegate_file_system::JSDelegateFileSystem; use crate::core::requests::config_request::ConfigRequest; use crate::core::requests::request_api::js_request_api::JSRequestApi; From 9e1d121faf1a826142c178b79474716e94ba1f3e Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Mon, 22 Apr 2024 15:47:50 +1000 Subject: [PATCH 14/26] Fix worker path being passed to memory fs --- .../core/core/test/requests/ConfigRequest.test.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index 12b00c09170..bd6b94fa863 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -28,9 +28,17 @@ async function assertThrows(block: () => Promise) { describe('ConfigRequest tests', () => { const projectRoot = '/project_root/'; - let fs = new MemoryFS(new WorkerFarm()); + let fs = new MemoryFS( + new WorkerFarm({ + workerPath: require.resolve('../../src/worker.js'), + }), + ); beforeEach(() => { - fs = new MemoryFS(new WorkerFarm()); + fs = new MemoryFS( + new WorkerFarm({ + workerPath: require.resolve('../../src/worker.js'), + }), + ); }); const getMockRunApi = ( From 33387e0ec7f970550b789ebc3be2cbc3011cb40d Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Mon, 22 Apr 2024 17:16:39 +1000 Subject: [PATCH 15/26] Fix test that was not running --- .../eslint-plugin/test/rules/no-self-package-imports.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dev/eslint-plugin/test/rules/no-self-package-imports.test.js b/packages/dev/eslint-plugin/test/rules/no-self-package-imports.test.js index d62013b131a..a8f43b91f73 100644 --- a/packages/dev/eslint-plugin/test/rules/no-self-package-imports.test.js +++ b/packages/dev/eslint-plugin/test/rules/no-self-package-imports.test.js @@ -9,7 +9,7 @@ const message = const filename = __filename; new RuleTester({ - parser: '@babel/eslint-parser', + parser: require.resolve('@babel/eslint-parser'), parserOptions: {ecmaVersion: 2018, sourceType: 'module'}, }).run('no-self-package-imports', rule, { valid: [ From 48ee00487c252ca351bfb9c222d45b685f136499 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 23 Apr 2024 09:09:52 +1000 Subject: [PATCH 16/26] Try to fix windows unit-tests --- .../core/core/test/requests/ConfigRequest.test.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index bd6b94fa863..0cbae09d79b 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -1,6 +1,7 @@ // @flow strict-local import WorkerFarm from '@parcel/workers'; +import path from 'path'; import assert from 'assert'; import sinon from 'sinon'; import {DEFAULT_FEATURE_FLAGS, setFeatureFlags} from '@parcel/feature-flags'; @@ -27,7 +28,7 @@ async function assertThrows(block: () => Promise) { } describe('ConfigRequest tests', () => { - const projectRoot = '/project_root/'; + const projectRoot = 'project_root'; let fs = new MemoryFS( new WorkerFarm({ workerPath: require.resolve('../../src/worker.js'), @@ -291,7 +292,10 @@ describe('ConfigRequest tests', () => { invalidateOnConfigKeyChange: [ { configKey: 'key1', - filePath: toProjectPath(projectRoot, '/project_root/config.json'), + filePath: toProjectPath( + projectRoot, + path.join('project_root', 'config.json'), + ), }, ], }); @@ -300,14 +304,14 @@ describe('ConfigRequest tests', () => { const fsCall = mockCast(fs).readFileSync.getCall(0); assert.deepEqual( fsCall?.args, - ['/project_root/config.json'], + [path.join('project_root', 'config.json')], 'readFile was called', ); } else { const fsCall = mockCast(fs).readFile.getCall(0); assert.deepEqual( fsCall?.args, - ['/project_root/config.json', 'utf8'], + [path.join('project_root', 'config.json'), 'utf8'], 'readFile was called', ); } From 0f939cd058eda011c5d891f0199e99907f89cc7c Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 24 Apr 2024 10:59:19 +1000 Subject: [PATCH 17/26] Update Cargo.lock --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 2cc5e6ace61..652f4f8c74b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -75,7 +75,7 @@ checksum = "7079075b41f533b8c61d2a4d073c4676e1f8b249ff94a393b0595db304e0dd87" name = "anyhow" version = "1.0.82" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4668cab20f66d8d020e1fbc0ebe47217433c1b6c8f2040faf858554e394ace6" +checksum = "f538837af36e6f6a9be0faa67f9a314f8119e4e4b5867c6ab40ed60360142519" dependencies = [ "backtrace", ] From 36ee4a87ef3a776f123cb324cbdab84dea9a4d84 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 24 Apr 2024 11:12:30 +1000 Subject: [PATCH 18/26] Fix broken CLIReporter tests --- .../reporters/cli/test/CLIReporter.test.js | 40 +++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/packages/reporters/cli/test/CLIReporter.test.js b/packages/reporters/cli/test/CLIReporter.test.js index 70d43eefe9b..46edd2911c4 100644 --- a/packages/reporters/cli/test/CLIReporter.test.js +++ b/packages/reporters/cli/test/CLIReporter.test.js @@ -11,6 +11,7 @@ import stripAnsi from 'strip-ansi'; import * as bundleReport from '../src/bundleReport'; import * as render from '../src/render'; import {DEFAULT_FEATURE_FLAGS} from '@parcel/feature-flags'; +import type {NamedBundle} from '@parcel/types-internal'; const EMPTY_OPTIONS = { cacheDir: '.parcel-cache', @@ -199,7 +200,7 @@ describe('CLIReporter', () => { // emit a buildSuccess event to reset the timings and seen phases // from the previous test process.env['PARCEL_SHOW_PHASE_TIMES'] = undefined; - // $FlowFixMe[incompatible-call] + // $FlowFixMe await _report({type: 'buildSuccess'}, EMPTY_OPTIONS); process.env['PARCEL_SHOW_PHASE_TIMES'] = 'true'; @@ -208,9 +209,18 @@ describe('CLIReporter', () => { EMPTY_OPTIONS, ); await _report({type: 'buildProgress', phase: 'bundling'}, EMPTY_OPTIONS); - // $FlowFixMe[incompatible-call] - await _report({type: 'buildProgress', phase: 'packaging'}, EMPTY_OPTIONS); - // $FlowFixMe[incompatible-call] + await _report( + // $FlowFixMe + { + type: 'buildProgress', + phase: 'packaging', + bundle: { + displayName: 'test', + }, + }, + EMPTY_OPTIONS, + ); + // $FlowFixMe await _report({type: 'buildSuccess'}, EMPTY_OPTIONS); const expected = /Building...\nBundling...\nPackaging & Optimizing...\nTransforming finished in [0-9]ms\nBundling finished in [0-9]ms\nPackaging & Optimizing finished in [0-9]ms/; @@ -224,10 +234,24 @@ describe('CLIReporter', () => { EMPTY_OPTIONS, ); await _report({type: 'buildProgress', phase: 'bundling'}, EMPTY_OPTIONS); - // $FlowFixMe[incompatible-call] - await _report({type: 'buildProgress', phase: 'packaging'}, EMPTY_OPTIONS); - // $FlowFixMe[incompatible-call] + await _report( + // $FlowFixMe + { + type: 'buildProgress', + phase: 'packaging', + bundle: { + displayName: 'test', + }, + }, + EMPTY_OPTIONS, + ); + // $FlowFixMe await _report({type: 'buildSuccess'}, EMPTY_OPTIONS); - assert.equal(expected.test(stdoutOutput), true); + + assert.equal( + expected.test(stdoutOutput), + true, + 'STDOUT output did not match', + ); }); }); From d5984ccf972a76fdca34dbe5dc9a846638921c37 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 24 Apr 2024 13:34:03 +1000 Subject: [PATCH 19/26] Fix CLI reporter tests --- packages/reporters/cli/test/CLIReporter.test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/reporters/cli/test/CLIReporter.test.js b/packages/reporters/cli/test/CLIReporter.test.js index 46edd2911c4..17d8bb2e467 100644 --- a/packages/reporters/cli/test/CLIReporter.test.js +++ b/packages/reporters/cli/test/CLIReporter.test.js @@ -4,14 +4,13 @@ import assert from 'assert'; import sinon from 'sinon'; import {PassThrough} from 'stream'; import {_report} from '../src/CLIReporter'; +import * as render from '../src/render'; import {_setStdio} from '../src/render'; import {inputFS, outputFS} from '@parcel/test-utils'; import {NodePackageManager} from '@parcel/package-manager'; import stripAnsi from 'strip-ansi'; import * as bundleReport from '../src/bundleReport'; -import * as render from '../src/render'; import {DEFAULT_FEATURE_FLAGS} from '@parcel/feature-flags'; -import type {NamedBundle} from '@parcel/types-internal'; const EMPTY_OPTIONS = { cacheDir: '.parcel-cache', From c096d34b82687b611b21dd51ea0ca952eaf627a2 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Tue, 30 Apr 2024 12:37:12 +1000 Subject: [PATCH 20/26] Update node-bindings formatting --- .../src/core/filesystem/in_memory_file_system/mod.rs | 7 +++++-- .../src/core/filesystem/js_delegate_file_system/mod.rs | 7 ++++--- .../node-bindings/src/core/requests/config_request/mod.rs | 4 +--- crates/node-bindings/src/core/requests/mod.rs | 7 ++++++- .../src/core/requests/request_api/js_request_api.rs | 7 +++++-- crates/node-bindings/src/core/requests/request_api/mod.rs | 3 ++- 6 files changed, 23 insertions(+), 12 deletions(-) diff --git a/crates/node-bindings/src/core/filesystem/in_memory_file_system/mod.rs b/crates/node-bindings/src/core/filesystem/in_memory_file_system/mod.rs index 13353a2fbda..cd2c8ea15d0 100644 --- a/crates/node-bindings/src/core/filesystem/in_memory_file_system/mod.rs +++ b/crates/node-bindings/src/core/filesystem/in_memory_file_system/mod.rs @@ -1,6 +1,9 @@ -use parcel_resolver::FileSystem; use std::collections::HashMap; -use std::path::{Component, Path, PathBuf}; +use std::path::Component; +use std::path::Path; +use std::path::PathBuf; + +use parcel_resolver::FileSystem; /// In memory implementation of a file-system entry enum InMemoryFileSystemEntry { diff --git a/crates/node-bindings/src/core/filesystem/js_delegate_file_system/mod.rs b/crates/node-bindings/src/core/filesystem/js_delegate_file_system/mod.rs index 6f9b82559a5..c0eb90c0f70 100644 --- a/crates/node-bindings/src/core/filesystem/js_delegate_file_system/mod.rs +++ b/crates/node-bindings/src/core/filesystem/js_delegate_file_system/mod.rs @@ -1,10 +1,11 @@ -use std::path::{Path, PathBuf}; +use std::path::Path; +use std::path::PathBuf; use std::rc::Rc; use dashmap::DashMap; use napi::bindgen_prelude::FromNapiValue; -use napi::{Env, JsObject}; - +use napi::Env; +use napi::JsObject; use parcel_resolver::FileSystem; use crate::core::requests::call_method; diff --git a/crates/node-bindings/src/core/requests/config_request/mod.rs b/crates/node-bindings/src/core/requests/config_request/mod.rs index 9948e76b35c..430da2c5e7a 100644 --- a/crates/node-bindings/src/core/requests/config_request/mod.rs +++ b/crates/node-bindings/src/core/requests/config_request/mod.rs @@ -5,7 +5,6 @@ use std::path::Path; use napi_derive::napi; - use parcel_resolver::FileSystem; use crate::core::requests::request_api::RequestApi; @@ -158,12 +157,11 @@ struct RequestOptions {} mod test { use parcel_resolver::OsFileSystem; + use super::*; use crate::core::filesystem::in_memory_file_system::InMemoryFileSystem; use crate::core::requests::config_request::run_config_request; use crate::core::requests::request_api::MockRequestApi; - use super::*; - #[test] fn test_run_empty_config_request_does_nothing() { let config_request = ConfigRequest { diff --git a/crates/node-bindings/src/core/requests/mod.rs b/crates/node-bindings/src/core/requests/mod.rs index f81249f670b..51a3dae94b4 100644 --- a/crates/node-bindings/src/core/requests/mod.rs +++ b/crates/node-bindings/src/core/requests/mod.rs @@ -1,7 +1,12 @@ use std::rc::Rc; use napi::bindgen_prelude::FromNapiValue; -use napi::{Env, JsFunction, JsObject, JsString, JsUnknown, NapiRaw}; +use napi::Env; +use napi::JsFunction; +use napi::JsObject; +use napi::JsString; +use napi::JsUnknown; +use napi::NapiRaw; use napi_derive::napi; use crate::core::filesystem::js_delegate_file_system::JSDelegateFileSystem; diff --git a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs index 83b5035f15d..a6a76356c7e 100644 --- a/crates/node-bindings/src/core/requests/request_api/js_request_api.rs +++ b/crates/node-bindings/src/core/requests/request_api/js_request_api.rs @@ -1,11 +1,14 @@ use std::path::Path; use std::rc::Rc; -use napi::{Env, JsObject, JsUnknown}; +use napi::Env; +use napi::JsObject; +use napi::JsUnknown; use crate::core::requests::call_method; use crate::core::requests::config_request::InternalFileCreateInvalidation; -use crate::core::requests::request_api::{RequestApi, RequestApiResult}; +use crate::core::requests::request_api::RequestApi; +use crate::core::requests::request_api::RequestApiResult; pub struct JSRequestApi { // TODO: Make sure it is safe to hold the environment like this diff --git a/crates/node-bindings/src/core/requests/request_api/mod.rs b/crates/node-bindings/src/core/requests/request_api/mod.rs index 74679fac6db..ddaa2eb4894 100644 --- a/crates/node-bindings/src/core/requests/request_api/mod.rs +++ b/crates/node-bindings/src/core/requests/request_api/mod.rs @@ -1,6 +1,7 @@ -use mockall::automock; use std::path::Path; +use mockall::automock; + use crate::core::requests::config_request::InternalFileCreateInvalidation; pub mod js_request_api; From 4bfff78819fd69b0b6a34256d6fe9e3007272e9f Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 1 May 2024 10:18:33 +1000 Subject: [PATCH 21/26] Try to fix adjacency list test timeouts --- .../core/graph/test/AdjacencyList.test.js | 21 +++++++++++++------ .../adjacency-list-shared-array.js | 4 +++- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/core/graph/test/AdjacencyList.test.js b/packages/core/graph/test/AdjacencyList.test.js index 9d320819381..2da7c4e02d5 100644 --- a/packages/core/graph/test/AdjacencyList.test.js +++ b/packages/core/graph/test/AdjacencyList.test.js @@ -273,7 +273,20 @@ describe('AdjacencyList', () => { }); describe('deserialize', function () { - this.timeout(10000); + this.timeout(20000); + + let worker; + beforeEach(done => { + worker = new Worker( + path.join(__dirname, 'integration/adjacency-list-shared-array.js'), + ); + let hasCalled = false; + worker.on('message', () => { + if (hasCalled) return; + hasCalled = true; + done(); + }); + }); it('should share the underlying data across worker threads', async () => { let graph = new AdjacencyList(); @@ -282,17 +295,13 @@ describe('AdjacencyList', () => { graph.addEdge(n0, n1, 1); graph.addEdge(n0, n1, 2); - let worker = new Worker( - path.join(__dirname, 'integration/adjacency-list-shared-array.js'), - ); - let originalSerialized = graph.serialize(); let originalNodes = [...originalSerialized.nodes]; let originalEdges = [...originalSerialized.edges]; let work = new Promise(resolve => worker.on('message', resolve)); worker.postMessage(originalSerialized); let received = AdjacencyList.deserialize(await work); - await worker.terminate(); + worker.terminate(); assert.deepEqual(received.serialize().nodes, graph.serialize().nodes); assert.deepEqual(received.serialize().edges, graph.serialize().edges); diff --git a/packages/core/graph/test/integration/adjacency-list-shared-array.js b/packages/core/graph/test/integration/adjacency-list-shared-array.js index 0c3460f92a8..94432c529ac 100644 --- a/packages/core/graph/test/integration/adjacency-list-shared-array.js +++ b/packages/core/graph/test/integration/adjacency-list-shared-array.js @@ -6,7 +6,7 @@ const { EdgeTypeMap, } = require('../../src/AdjacencyList'); -parentPort.once('message', (serialized) => { +parentPort.once('message', serialized => { let graph = AdjacencyList.deserialize(serialized); serialized.nodes.forEach((v, i) => { if (i < NodeTypeMap.HEADER_SIZE) return; @@ -18,3 +18,5 @@ parentPort.once('message', (serialized) => { }); parentPort.postMessage(graph.serialize()); }); + +parentPort.postMessage('ready'); From b257eb323c7f405f07d620827265e40c69a994b6 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 1 May 2024 10:20:07 +1000 Subject: [PATCH 22/26] Make changes look better --- .../core/graph/test/AdjacencyList.test.js | 19 +++++-------------- .../adjacency-list-shared-array.js | 2 -- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/packages/core/graph/test/AdjacencyList.test.js b/packages/core/graph/test/AdjacencyList.test.js index 2da7c4e02d5..aadb1303d49 100644 --- a/packages/core/graph/test/AdjacencyList.test.js +++ b/packages/core/graph/test/AdjacencyList.test.js @@ -275,19 +275,6 @@ describe('AdjacencyList', () => { describe('deserialize', function () { this.timeout(20000); - let worker; - beforeEach(done => { - worker = new Worker( - path.join(__dirname, 'integration/adjacency-list-shared-array.js'), - ); - let hasCalled = false; - worker.on('message', () => { - if (hasCalled) return; - hasCalled = true; - done(); - }); - }); - it('should share the underlying data across worker threads', async () => { let graph = new AdjacencyList(); let n0 = graph.addNode(); @@ -295,13 +282,17 @@ describe('AdjacencyList', () => { graph.addEdge(n0, n1, 1); graph.addEdge(n0, n1, 2); + let worker = new Worker( + path.join(__dirname, 'integration/adjacency-list-shared-array.js'), + ); + let originalSerialized = graph.serialize(); let originalNodes = [...originalSerialized.nodes]; let originalEdges = [...originalSerialized.edges]; let work = new Promise(resolve => worker.on('message', resolve)); worker.postMessage(originalSerialized); let received = AdjacencyList.deserialize(await work); - worker.terminate(); + await worker.terminate(); assert.deepEqual(received.serialize().nodes, graph.serialize().nodes); assert.deepEqual(received.serialize().edges, graph.serialize().edges); diff --git a/packages/core/graph/test/integration/adjacency-list-shared-array.js b/packages/core/graph/test/integration/adjacency-list-shared-array.js index 94432c529ac..174e7473238 100644 --- a/packages/core/graph/test/integration/adjacency-list-shared-array.js +++ b/packages/core/graph/test/integration/adjacency-list-shared-array.js @@ -18,5 +18,3 @@ parentPort.once('message', serialized => { }); parentPort.postMessage(graph.serialize()); }); - -parentPort.postMessage('ready'); From 8c3a0d994170476a6d1f23aee6961fa06d3e10af Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 1 May 2024 10:49:30 +1000 Subject: [PATCH 23/26] Increase timeout to 30s --- packages/core/graph/test/AdjacencyList.test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/core/graph/test/AdjacencyList.test.js b/packages/core/graph/test/AdjacencyList.test.js index aadb1303d49..666a7e89eaa 100644 --- a/packages/core/graph/test/AdjacencyList.test.js +++ b/packages/core/graph/test/AdjacencyList.test.js @@ -273,7 +273,7 @@ describe('AdjacencyList', () => { }); describe('deserialize', function () { - this.timeout(20000); + this.timeout(30000); it('should share the underlying data across worker threads', async () => { let graph = new AdjacencyList(); @@ -292,7 +292,8 @@ describe('AdjacencyList', () => { let work = new Promise(resolve => worker.on('message', resolve)); worker.postMessage(originalSerialized); let received = AdjacencyList.deserialize(await work); - await worker.terminate(); + // eslint-disable-next-line no-unused-vars + const _terminatePromise = worker.terminate(); assert.deepEqual(received.serialize().nodes, graph.serialize().nodes); assert.deepEqual(received.serialize().edges, graph.serialize().edges); From a52a7bd13607159e4a6ad1c0e87e67fc0e31e2e3 Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 1 May 2024 10:58:00 +1000 Subject: [PATCH 24/26] Start less workers --- .../core/test/requests/ConfigRequest.test.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/core/core/test/requests/ConfigRequest.test.js b/packages/core/core/test/requests/ConfigRequest.test.js index 0cbae09d79b..c91964dba7d 100644 --- a/packages/core/core/test/requests/ConfigRequest.test.js +++ b/packages/core/core/test/requests/ConfigRequest.test.js @@ -29,17 +29,13 @@ async function assertThrows(block: () => Promise) { describe('ConfigRequest tests', () => { const projectRoot = 'project_root'; - let fs = new MemoryFS( - new WorkerFarm({ - workerPath: require.resolve('../../src/worker.js'), - }), - ); + const farm = new WorkerFarm({ + workerPath: require.resolve('../../src/worker.js'), + maxConcurrentWorkers: 1, + }); + let fs = new MemoryFS(farm); beforeEach(() => { - fs = new MemoryFS( - new WorkerFarm({ - workerPath: require.resolve('../../src/worker.js'), - }), - ); + fs = new MemoryFS(farm); }); const getMockRunApi = ( From fee2b2974984aaab98ed2a0dda5e2e2ebf2b5cae Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 1 May 2024 10:59:10 +1000 Subject: [PATCH 25/26] Revert timeout to where it was --- packages/core/graph/test/AdjacencyList.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/graph/test/AdjacencyList.test.js b/packages/core/graph/test/AdjacencyList.test.js index 666a7e89eaa..acef303f3d6 100644 --- a/packages/core/graph/test/AdjacencyList.test.js +++ b/packages/core/graph/test/AdjacencyList.test.js @@ -273,7 +273,7 @@ describe('AdjacencyList', () => { }); describe('deserialize', function () { - this.timeout(30000); + this.timeout(10000); it('should share the underlying data across worker threads', async () => { let graph = new AdjacencyList(); From c3603df3e2c4f7f5dc6c9ea9b1dd0346bf011d1e Mon Sep 17 00:00:00 2001 From: Pedro Yamada Date: Wed, 1 May 2024 11:28:46 +1000 Subject: [PATCH 26/26] Update the CI workflow --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46e9ff57702..3b0e3cc1aaf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,6 +1,7 @@ name: Continuous Integration on: + merge_group: pull_request: push: branches: