From e2c0a87d84504a93abbbdde573391dace947d898 Mon Sep 17 00:00:00 2001 From: Ashley Michal Lewis Date: Fri, 13 Dec 2019 14:42:04 -0600 Subject: [PATCH 1/8] Step 1: don't restrict custom webpack config for sites --- src/commands/build/wranglerjs/mod.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/commands/build/wranglerjs/mod.rs b/src/commands/build/wranglerjs/mod.rs index 5b168576c..751bad671 100644 --- a/src/commands/build/wranglerjs/mod.rs +++ b/src/commands/build/wranglerjs/mod.rs @@ -175,19 +175,11 @@ fn setup_build(target: &Target) -> Result<(Command, PathBuf, Bundle), failure::E command.arg(format!("--wasm-binding={}", bundle.get_wasm_binding())); let custom_webpack_config_path = match &target.webpack_config { - Some(webpack_config) => match &target.site { - None => Some(PathBuf::from(&webpack_config)), - Some(_) => { - message::warn("Workers Sites does not support custom webpack configuration files"); - None - } - }, + Some(webpack_config) => Some(PathBuf::from(&webpack_config)), None => { - if target.site.is_none() { - let config_path = PathBuf::from("webpack.config.js".to_string()); - if config_path.exists() { - message::warn("If you would like to use your own custom webpack configuration, you will need to add this to your wrangler.toml:\nwebpack_config = \"webpack.config.js\""); - } + let config_path = PathBuf::from("webpack.config.js".to_string()); + if config_path.exists() { + message::warn("If you would like to use your own custom webpack configuration, you will need to add this to your wrangler.toml:\nwebpack_config = \"webpack.config.js\""); } None } From 9630a864f99801c4190fe01a64f5b2cfb2b4f7fa Mon Sep 17 00:00:00 2001 From: Ashley Michal Lewis Date: Fri, 13 Dec 2019 16:08:52 -0600 Subject: [PATCH 2/8] Rename webpack_no_config -> webpack_build --- tests/build.rs | 6 +++--- tests/fixture/wrangler_toml.rs | 6 +++--- tests/preview.rs | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/build.rs b/tests/build.rs index bec6b31c5..2ebcd0837 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -14,7 +14,7 @@ fn it_builds_webpack() { let fixture = Fixture::new(); fixture.scaffold_webpack(); - let wrangler_toml = WranglerToml::webpack_no_config("test-build-webpack"); + let wrangler_toml = WranglerToml::webpack_build("test-build-webpack"); fixture.create_wrangler_toml(wrangler_toml); build_creates_assets(&fixture, vec!["script.js"]); @@ -41,7 +41,7 @@ fn it_builds_with_webpack_single_js() { ); fixture.create_default_package_json(); - let wrangler_toml = WranglerToml::webpack_no_config("test-build-webpack-single-js"); + let wrangler_toml = WranglerToml::webpack_build("test-build-webpack-single-js"); fixture.create_wrangler_toml(wrangler_toml); build_creates_assets(&fixture, vec!["script.js"]); @@ -137,7 +137,7 @@ fn it_builds_with_webpack_single_js_missing_package_main() { ); let wrangler_toml = - WranglerToml::webpack_no_config("test-build-webpack-single-js-missing-package-main"); + WranglerToml::webpack_build("test-build-webpack-single-js-missing-package-main"); fixture.create_wrangler_toml(wrangler_toml); build_fails_with( diff --git a/tests/fixture/wrangler_toml.rs b/tests/fixture/wrangler_toml.rs index bbf23ff29..097dc6343 100644 --- a/tests/fixture/wrangler_toml.rs +++ b/tests/fixture/wrangler_toml.rs @@ -57,7 +57,7 @@ pub struct WranglerToml<'a> { } impl WranglerToml<'_> { - pub fn webpack_no_config(name: &str) -> WranglerToml { + pub fn webpack_build(name: &str) -> WranglerToml { let mut wrangler_toml = WranglerToml::default(); wrangler_toml.name = Some(name); wrangler_toml.workers_dev = Some(true); @@ -67,14 +67,14 @@ impl WranglerToml<'_> { } pub fn webpack_std_config(name: &str) -> WranglerToml { - let mut wrangler_toml = WranglerToml::webpack_no_config(name); + let mut wrangler_toml = WranglerToml::webpack_build(name); wrangler_toml.webpack_config = Some("webpack.config.js"); wrangler_toml } pub fn webpack_custom_config<'a>(name: &'a str, webpack_config: &'a str) -> WranglerToml<'a> { - let mut wrangler_toml = WranglerToml::webpack_no_config(name); + let mut wrangler_toml = WranglerToml::webpack_build(name); wrangler_toml.webpack_config = Some(webpack_config); wrangler_toml diff --git a/tests/preview.rs b/tests/preview.rs index b01d98656..06a920234 100644 --- a/tests/preview.rs +++ b/tests/preview.rs @@ -43,7 +43,7 @@ fn it_can_preview_webpack_project() { let fixture = Fixture::new(); fixture.scaffold_webpack(); - let wrangler_toml = WranglerToml::webpack_no_config("test-preview-webpack"); + let wrangler_toml = WranglerToml::webpack_build("test-preview-webpack"); fixture.create_wrangler_toml(wrangler_toml); preview_succeeds(&fixture); From 06fb229fe3b6351e7fcc7f58cd8e94b2db0133b9 Mon Sep 17 00:00:00 2001 From: Ashley Michal Lewis Date: Fri, 13 Dec 2019 16:10:01 -0600 Subject: [PATCH 3/8] add output_path field to fixture struct --- tests/fixture/mod.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/fixture/mod.rs b/tests/fixture/mod.rs index 7013afad3..d3f8efbda 100644 --- a/tests/fixture/mod.rs +++ b/tests/fixture/mod.rs @@ -14,25 +14,27 @@ use toml; const BUNDLE_OUT: &str = "worker"; -pub struct Fixture { +pub struct Fixture<'a> { // we wrap the fixture's tempdir in a `ManuallyDrop` so that if a test // fails, its directory isn't deleted, and we have a chance to manually // inspect its state and figure out what is going on. dir: ManuallyDrop, + output_path: &'a str, } -impl Default for Fixture { +impl Default for Fixture<'_> { fn default() -> Self { Self::new() } } -impl Fixture { - pub fn new() -> Fixture { +impl Fixture<'_> { + pub fn new() -> Fixture<'static> { let dir = TempDir::new().unwrap(); eprintln!("Created fixture at {}", dir.path().display()); Fixture { dir: ManuallyDrop::new(dir), + output_path: BUNDLE_OUT, } } @@ -46,7 +48,7 @@ impl Fixture { } pub fn get_output_path(&self) -> PathBuf { - self.get_path().join(BUNDLE_OUT) + self.get_path().join(self.output_path) } pub fn create_file(&self, name: &str, content: &str) { @@ -91,7 +93,7 @@ impl Fixture { } } -impl Drop for Fixture { +impl Drop for Fixture<'_> { fn drop(&mut self) { if !thread::panicking() { unsafe { ManuallyDrop::drop(&mut self.dir) } From 3fee9539c3a7641c6b1e25c9000bce503ee7160c Mon Sep 17 00:00:00 2001 From: Ashley Michal Lewis Date: Fri, 13 Dec 2019 16:11:52 -0600 Subject: [PATCH 4/8] add test, fixtures for it builds site (known control) --- tests/build.rs | 10 ++++++++++ tests/fixture/mod.rs | 26 ++++++++++++++++++++++++++ tests/fixture/wrangler_toml.rs | 9 +++++++++ 3 files changed, 45 insertions(+) diff --git a/tests/build.rs b/tests/build.rs index 2ebcd0837..8c2676ffe 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -20,6 +20,16 @@ fn it_builds_webpack() { build_creates_assets(&fixture, vec!["script.js"]); } +#[test] +fn it_builds_webpack_site() { + let fixture = Fixture::new_site(); + + let wrangler_toml = WranglerToml::site("test-build-site"); + fixture.create_wrangler_toml(wrangler_toml); + + build_creates_assets(&fixture, vec!["main.js"]); +} + #[test] fn it_builds_with_webpack_single_js() { let fixture = Fixture::new(); diff --git a/tests/fixture/mod.rs b/tests/fixture/mod.rs index d3f8efbda..09bfeadae 100644 --- a/tests/fixture/mod.rs +++ b/tests/fixture/mod.rs @@ -38,6 +38,15 @@ impl Fixture<'_> { } } + pub fn new_site() -> Fixture<'static> { + let mut fixture = Fixture::new(); + fixture.output_path = "dist"; + + fixture.scaffold_site(); + + fixture + } + pub fn get_path(&self) -> PathBuf { self.dir.path().to_path_buf() } @@ -82,6 +91,23 @@ impl Fixture<'_> { self.create_file("wrangler.toml", &toml::to_string(&wrangler_toml).unwrap()); } + pub fn scaffold_site(&self) { + self.create_dir("workers-site"); + self.create_file( + "workers-site/package.json", + r#" + { + "private": true, + "main": "index.js", + "dependencies": { + "@cloudflare/kv-asset-handler": "^0.0.5" + } + } + "#, + ); + self.create_file("workers-site/index.js", ""); + } + pub fn lock(&self) -> MutexGuard<'static, ()> { use std::sync::Mutex; diff --git a/tests/fixture/wrangler_toml.rs b/tests/fixture/wrangler_toml.rs index 097dc6343..0f8e46215 100644 --- a/tests/fixture/wrangler_toml.rs +++ b/tests/fixture/wrangler_toml.rs @@ -97,4 +97,13 @@ impl WranglerToml<'_> { wrangler_toml } + + pub fn site(name: &str) -> WranglerToml { + let mut wrangler_toml = WranglerToml::webpack_build(name); + let mut site = SiteConfig::default(); + site.bucket = Some("./public"); + wrangler_toml.site = Some(site); + + wrangler_toml + } } From ebbb8b2be5c6ba8e276c9e3f86e60bc2273f0958 Mon Sep 17 00:00:00 2001 From: Ashley Michal Lewis Date: Fri, 13 Dec 2019 16:29:20 -0600 Subject: [PATCH 5/8] add test for building workers site with custom webpack --- tests/build.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/build.rs b/tests/build.rs index 8c2676ffe..c377a1d90 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -30,6 +30,24 @@ fn it_builds_webpack_site() { build_creates_assets(&fixture, vec!["main.js"]); } +#[test] +fn it_builds_webpack_site_with_custom_webpack() { + let fixture = Fixture::new_site(); + + fixture.create_file( + "workers-site/webpack.worker.js", + r#" + module.exports = { context: __dirname, entry: "./index.js" }; + "#, + ); + + let mut wrangler_toml = WranglerToml::site("test-build-site-specify-config"); + wrangler_toml.webpack_config = Some("workers-site/webpack.worker.js"); + fixture.create_wrangler_toml(wrangler_toml); + + build_creates_assets(&fixture, vec!["main.js"]); +} + #[test] fn it_builds_with_webpack_single_js() { let fixture = Fixture::new(); From db3ab6049889a106c210d44a3184f9c49da2314e Mon Sep 17 00:00:00 2001 From: Ashley Michal Lewis Date: Mon, 16 Dec 2019 15:33:58 -0600 Subject: [PATCH 6/8] Use wrangler's output rather than dist/ for tests --- tests/build.rs | 6 +++--- tests/fixture/mod.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/build.rs b/tests/build.rs index c377a1d90..d6b99c649 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -27,7 +27,7 @@ fn it_builds_webpack_site() { let wrangler_toml = WranglerToml::site("test-build-site"); fixture.create_wrangler_toml(wrangler_toml); - build_creates_assets(&fixture, vec!["main.js"]); + build_creates_assets(&fixture, vec!["script.js"]); } #[test] @@ -37,7 +37,7 @@ fn it_builds_webpack_site_with_custom_webpack() { fixture.create_file( "workers-site/webpack.worker.js", r#" - module.exports = { context: __dirname, entry: "./index.js" }; + module.exports = { entry: "./workers-site/index.js" }; "#, ); @@ -45,7 +45,7 @@ fn it_builds_webpack_site_with_custom_webpack() { wrangler_toml.webpack_config = Some("workers-site/webpack.worker.js"); fixture.create_wrangler_toml(wrangler_toml); - build_creates_assets(&fixture, vec!["main.js"]); + build_creates_assets(&fixture, vec!["script.js"]); } #[test] diff --git a/tests/fixture/mod.rs b/tests/fixture/mod.rs index 09bfeadae..fbd058846 100644 --- a/tests/fixture/mod.rs +++ b/tests/fixture/mod.rs @@ -40,7 +40,7 @@ impl Fixture<'_> { pub fn new_site() -> Fixture<'static> { let mut fixture = Fixture::new(); - fixture.output_path = "dist"; + fixture.output_path = "workers-site/worker"; fixture.scaffold_site(); From 4bd3843278e1a91ad3af86d26358931b7f056eeb Mon Sep 17 00:00:00 2001 From: Ashley Michal Lewis Date: Tue, 17 Dec 2019 15:32:34 -0600 Subject: [PATCH 7/8] Better error message when custom webpack fails --- src/commands/build/wranglerjs/mod.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/commands/build/wranglerjs/mod.rs b/src/commands/build/wranglerjs/mod.rs index 751bad671..2cac8135d 100644 --- a/src/commands/build/wranglerjs/mod.rs +++ b/src/commands/build/wranglerjs/mod.rs @@ -46,7 +46,8 @@ pub fn run_build(target: &Target) -> Result<(), failure::Error> { let wranglerjs_output: WranglerjsOutput = serde_json::from_str(&output).expect("could not parse wranglerjs output"); - write_wranglerjs_output(&bundle, &wranglerjs_output) + let custom_webpack = target.webpack_config.is_some(); + write_wranglerjs_output(&bundle, &wranglerjs_output, custom_webpack) } else { failure::bail!("failed to execute `{:?}`: exited with {}", command, status) } @@ -57,6 +58,7 @@ pub fn run_build_and_watch(target: &Target, tx: Option>) -> Result<() command.arg("--watch=1"); let is_site = target.site.clone(); + let custom_webpack = target.webpack_config.is_some(); log::info!("Running {:?} in watch mode", command); @@ -100,7 +102,8 @@ pub fn run_build_and_watch(target: &Target, tx: Option>) -> Result<() let wranglerjs_output: WranglerjsOutput = serde_json::from_str(&output).expect("could not parse wranglerjs output"); - if write_wranglerjs_output(&bundle, &wranglerjs_output).is_ok() { + if write_wranglerjs_output(&bundle, &wranglerjs_output, custom_webpack).is_ok() + { if let Some(tx) = tx.clone() { tx.send(()).expect("--watch change message failed to send"); } @@ -117,12 +120,19 @@ pub fn run_build_and_watch(target: &Target, tx: Option>) -> Result<() fn write_wranglerjs_output( bundle: &Bundle, output: &WranglerjsOutput, + custom_webpack: bool, ) -> Result<(), failure::Error> { if output.has_errors() { message::user_error(output.get_errors().as_str()); - failure::bail!( - "webpack returned an error. You may be able to resolve this issue by running npm install." + if custom_webpack { + failure::bail!( + "Webpack returned an error. Try configuring `entry` in your webpack config relative to the current working directory, or setting `context = __dirname` in your webpack config." ); + } else { + failure::bail!( + "Webpack returned an error. You may be able to resolve this issue by running npm install." + ); + } } bundle.write(output)?; From b01909a9dd7185aa74728a1867f12a937300274b Mon Sep 17 00:00:00 2001 From: Ashley Michal Date: Tue, 17 Dec 2019 15:33:56 -0600 Subject: [PATCH 8/8] Apply suggestions from code review Co-Authored-By: Sven Sauleau --- src/commands/build/wranglerjs/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/build/wranglerjs/mod.rs b/src/commands/build/wranglerjs/mod.rs index 2cac8135d..9022ae06f 100644 --- a/src/commands/build/wranglerjs/mod.rs +++ b/src/commands/build/wranglerjs/mod.rs @@ -126,11 +126,11 @@ fn write_wranglerjs_output( message::user_error(output.get_errors().as_str()); if custom_webpack { failure::bail!( - "Webpack returned an error. Try configuring `entry` in your webpack config relative to the current working directory, or setting `context = __dirname` in your webpack config." + "webpack returned an error. Try configuring `entry` in your webpack config relative to the current working directory, or setting `context = __dirname` in your webpack config." ); } else { failure::bail!( - "Webpack returned an error. You may be able to resolve this issue by running npm install." + "webpack returned an error. You may be able to resolve this issue by running npm install." ); } }