Apply suggestions from code review

- Check total bundle size, not individual files
- Replace hyphens with underscores in wrangler.toml
- Rename MAX_FILE_SIZE to MAX_BUNDLE_SIZE
- Duplicate comment about swc_common::SourceMap vs sourcemap::SourceMap
- rename entry & entry_file to module & module_path

Cargo.toml

@@ -15,7 +15,6 @@ atty = "0.2.14"
 base64 = "0.13.0"
 billboard = "0.1.0"
 binary-install = "0.0.3-alpha.1"
-bytesize = "1.0.1"
 chrome-devtools-rs = { version = "0.0.0-alpha.1", features = ["color"] }
 chrono = "0.4.19"
 clap = "2.33.3"

src/build/check/config.rs

@@ -1,4 +1,3 @@
-use bytesize::ByteSize;
 use swc_ecma_parser::{EsConfig, Syntax};
 use wasmparser::WasmFeatures;
 
@@ -36,14 +35,9 @@ pub const V8_SUPPORTED_JS_FEATURES: Syntax = Syntax::Es(EsConfig {
     optional_chaining: true,
     // https://v8.dev/features/modules#import-meta
     import_meta: true,
-    // ok so for top-level await, V8 says there's no support in
-    // "classic scripts", which i think is how workers are executed.
-    // i mean, they're certainly not modules.
     // https://v8.dev/features/top-level-await#new
-    top_level_await: false,
-    // i literally cannot find a source on this
-    // i don't...is it `console.assert?`
-    // TODO: ask in the slack about this one
+    top_level_await: true,
+    // https://bugs.chromium.org/p/v8/issues/detail?id=10958
     import_assertions: false,
 });
 
@@ -82,6 +76,6 @@ pub const V8_SUPPORTED_WASM_FEATURES: WasmFeatures = WasmFeatures {
     memory64: false,
 };
 
-/// The [maximum file size](https://developers.cloudflare.com/workers/platform/limits#script-size) we allow.
+/// We allow a [maximum bundle size](https://developers.cloudflare.com/workers/platform/limits#script-size) of 1MiB.
 #[doc(inline)]
-pub const MAX_FILE_SIZE: ByteSize = ByteSize(1_000_000);
+pub const MAX_BUNDLE_SIZE: u64 = 1 << 20;

src/build/check/js/mod.rs

@@ -1,11 +1,9 @@
-use std::path::PathBuf;
-
 use sourcemap::SourceMap;
 use swc_common::{sync::Lrc, SourceMap as SwcSourceMap};
 use swc_ecma_ast::{ImportDecl, Module};
 use swc_ecma_visit::{Node, Visit, VisitWith};
 
-use super::{config::V8_SUPPORTED_JS_FEATURES, Lintable, Parseable, Validate};
+use super::{config::V8_SUPPORTED_JS_FEATURES, Lintable, Parseable};
 
 // bring implemntations of Lintable and Parseable into scope
 mod lint;
@@ -14,13 +12,15 @@ mod parse;
 /// A representation of a JS file (+ an optional source map)
 /// produced by a bundler's output
 pub struct JavaScript {
+    /// A JavaScript module, as parsed by SWC
     module: Module,
+    /// SWC's `SourceMap` struct refers to an in-memory representation of the JS,
+    /// and has nothing to do with actual javascript source maps
     swc_source_map: Lrc<SwcSourceMap>,
+    /// This, on the other hand, is a javascript source map
     js_source_map: Option<SourceMap>,
 }
 
-impl Validate<PathBuf> for JavaScript {}
-
 impl std::fmt::Debug for JavaScript {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("JavaScript")

src/build/check/js/parse.rs

@@ -18,15 +18,7 @@ impl Parseable<PathBuf> for JavaScript {
         // TODO ask in the slack
         let mut parser = Parser::new(V8_SUPPORTED_JS_FEATURES, StringInput::from(&*fm), None);
 
-        // TODO: need feedback
-        // ok so these errors are recoverable, like we can successfully parse it.
-        // if we wanted to be stricter, we could just Err here
-        // we could also warn the user about these, but i think
-        // we should first do some testing and figure out what level
-        // of verbosity is appropriate.
-        // my guess is that the V8 parser is better at recovering than swc
-        // but i also know nothing about that. i just know google is a multi-billion
-        // dollar company and swc is one guy
+        // these errors are recoverable
         let _ = parser.take_errors();
 
         let js_extension = js_file.extension().unwrap().to_str().unwrap();
@@ -35,6 +27,7 @@ impl Parseable<PathBuf> for JavaScript {
         let js_source_map = if map.is_file() {
             Some(SourceMap::from_reader(File::open(map)?)?)
         } else {
+            // inline source map support when??
             None
         };
 

src/build/check/mod.rs

@@ -2,18 +2,21 @@ use std::{
     collections::hash_map::Entry,
     collections::HashMap,
     fmt::Debug,
+    fs::File,
+    io::{Read, Write},
     iter,
     path::{Path, PathBuf},
 };
 
+use flate2::{write::ZlibEncoder, Compression};
+use number_prefix::NumberPrefix;
+
 mod config;
 mod js;
-mod util;
 mod wasm;
 
-use self::config::MAX_FILE_SIZE;
+use self::config::MAX_BUNDLE_SIZE;
 use js::JavaScript;
-use util::*;
 use wasm::WebAssembly;
 
 const WORKER_FILE_NAME: &str = "worker.mjs";
@@ -70,10 +73,8 @@ pub trait Lintable {
     fn lint(&self) -> Result<(), failure::Error>;
 }
 
-/// If a struct is both Parseable and Lintable, then it may also implement
-/// Validate, which should serve as a one-stop-shop to go from un-parsed
-/// input to linted output. A default implmenetation is provided which
-/// simply calls `Self::parse(&input).lint()`
+/// If a struct is both Parseable and Lintable, then a blanket implementation
+/// of Validate is provided to combine the steps of Parsing and Linting.
 /// ```
 /// # trait Parseable<ParserInput>: Sized {
 /// #    fn parse(input: &ParserInput) -> Result<Self, failure::Error>;
@@ -83,12 +84,17 @@ pub trait Lintable {
 /// #     fn lint(&self) -> Result<(), failure::Error>;
 /// # }
 /// #
-/// pub trait Validate<ParserInput>:
-///     Lintable + Parseable<ParserInput>
+/// pub trait Validate<ParserInput>: Lintable + Parseable<ParserInput> {
+///     fn validate(input: ParserInput) -> Result<(), failure::Error>;
+/// }
+///
+/// impl<T, Input> Validate<Input> for T
+/// where
+///     T: Parseable<Input> + Lintable,
 /// {
-///         fn validate(input: ParserInput) -> Result<(), failure::Error> {
-///             let t = Self::parse(&input)?;
-///             t.lint()
+///     fn validate(input: Input) -> Result<(), failure::Error> {
+///         let t = Self::parse(&input)?;
+///         t.lint()
 ///     }
 /// }
 ///
@@ -110,13 +116,20 @@ pub trait Lintable {
 ///     }
 /// }
 ///
-/// impl Validate<u8> for SmallNumber {};
+/// // Validate is provided automatically!
 ///
 /// assert!(SmallNumber::validate(3).is_ok());
 /// assert!(SmallNumber::validate(6).is_err());
 /// ```
 pub trait Validate<ParserInput>: Lintable + Parseable<ParserInput> {
-    fn validate(input: ParserInput) -> Result<(), failure::Error> {
+    fn validate(input: ParserInput) -> Result<(), failure::Error>;
+}
+
+impl<T, Input> Validate<Input> for T
+where
+    T: Parseable<Input> + Lintable,
+{
+    fn validate(input: Input) -> Result<(), failure::Error> {
         let t = Self::parse(&input)?;
         t.lint()
     }
@@ -139,9 +152,9 @@ pub trait Validate<ParserInput>: Lintable + Parseable<ParserInput> {
 #[derive(Debug)]
 pub struct BundlerOutput {
     /// A PathBuf pointing to worker.mjs, the entrypoint of the worker
-    entry_file: PathBuf,
+    module_path: PathBuf,
     /// An in-memory representation of the worker entrypoint
-    entry: JavaScript,
+    module: JavaScript,
     /// Other JS files that are executed in the Worker
     javascript: HashMap<PathBuf, JavaScript>,
     /// WebAssembly that gets executed in the Worker
@@ -161,15 +174,15 @@ pub struct BundlerOutput {
 /// will not be added to the resulting BundlerOutput
 impl<P: AsRef<Path> + Debug> Parseable<P> for BundlerOutput {
     fn parse(output_dir: &P) -> Result<Self, failure::Error> {
-        let entry_file = output_dir.as_ref().join(WORKER_FILE_NAME);
-        let entry = JavaScript::parse(&entry_file)?;
+        let module_path = output_dir.as_ref().join(WORKER_FILE_NAME);
+        let module = JavaScript::parse(&module_path)?;
 
         let mut javascript = HashMap::new();
         let mut webassembly = HashMap::new();
         let mut other_files = vec![];
 
         // Create a stack of the imports in the worker entrypoint
-        let mut imports = entry.find_imports();
+        let mut imports = module.find_imports();
 
         // Work through the stack, adding more imports as necessary
         while let Some(import) = imports.pop() {
@@ -229,8 +242,8 @@ impl<P: AsRef<Path> + Debug> Parseable<P> for BundlerOutput {
         }
 
         Ok(Self {
-            entry_file,
-            entry,
+            module_path,
+            module,
             javascript,
             webassembly,
             other_files,
@@ -244,19 +257,48 @@ impl<P: AsRef<Path> + Debug> Parseable<P> for BundlerOutput {
 /// than are absolutely necessary for this PR
 impl Lintable for BundlerOutput {
     fn lint(&self) -> Result<(), failure::Error> {
-        // Check file sizes
-        iter::once(&self.entry_file)
+        // Check the bundle size
+        let mut encoder = ZlibEncoder::new(Vec::new(), Compression::default());
+        let mut buffer = Vec::new();
+
+        iter::once(&self.module_path)
             .chain(self.javascript.keys())
             .chain(self.webassembly.keys())
             .chain(&self.other_files)
-            .try_for_each(|file| check_file_size(file, MAX_FILE_SIZE))?;
+            .try_for_each(|path: &PathBuf| -> Result<(), failure::Error> {
+                let mut file = File::open(path)?;
+                file.read_to_end(&mut buffer)?;
+                Ok(())
+            })?;
+
+        encoder.write_all(&mut buffer)?;
+
+        let compressed_size = encoder.finish()?.len() as u64;
+        let human_compressed_size = match NumberPrefix::binary(compressed_size as f64) {
+            NumberPrefix::Standalone(bytes) => format!("{} bytes", bytes),
+            NumberPrefix::Prefixed(prefix, n) => format!("{:.0} {}B", n, prefix),
+        };
+
+        // i wish this was a const but
+        // "caLlS IN cOnSTAntS ArE LiMITed TO CoNstaNt fUNCtIoNs, TupLE sTrUCts aND TUpLE VaRiANTs"
+        // or whatever
+        let human_max_size = match NumberPrefix::binary(MAX_BUNDLE_SIZE as f64) {
+            NumberPrefix::Standalone(bytes) => format!("{} bytes", bytes),
+            NumberPrefix::Prefixed(prefix, n) => format!("{:.0} {}B", n, prefix),
+        };
+
+        // warn, but continue
+        if compressed_size > MAX_BUNDLE_SIZE {
+            println!(
+                "Your project ({}) has exceeded the {} limit and may fail to deploy.",
+                human_compressed_size, human_max_size
+            );
+        }
 
         // Lint the various files
-        iter::once(&self.entry as &dyn Lintable)
+        iter::once(&self.module as &dyn Lintable)
             .chain(self.javascript.values().map(|js| js as &dyn Lintable))
             .chain(self.webassembly.values().map(|wasm| wasm as &dyn Lintable))
             .try_for_each(|file| file.lint())
     }
 }
-
-impl<P: AsRef<Path> + Debug> Validate<P> for BundlerOutput {}

src/build/check/wasm.rs

@@ -2,7 +2,7 @@ use std::{fs::File, io::Read, path::PathBuf};
 
 use wasmparser::Validator;
 
-use super::{config::V8_SUPPORTED_WASM_FEATURES, Lintable, Parseable, Validate};
+use super::{config::V8_SUPPORTED_WASM_FEATURES, Lintable, Parseable};
 
 #[derive(Debug)]
 pub struct WebAssembly {
@@ -30,5 +30,3 @@ impl Lintable for WebAssembly {
         }
     }
 }
-
-impl Validate<(PathBuf, Option<PathBuf>)> for WebAssembly {}

src/settings/toml/bundle.rs

@@ -14,11 +14,13 @@ const BUILD_COMMAND: &str = "npm run build";
 #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
 #[serde(deny_unknown_fields)]
 pub struct Bundle {
-    #[serde(rename = "build-command")]
+    // are these renames necessary? I'm not super well versed in serde
+    // so i'm not sure how the annotations work
+    #[serde(rename = "build_command")]
     build_command: Option<String>,
-    #[serde(rename = "output-dir")]
+    #[serde(rename = "output_dir")]
     output_dir: Option<PathBuf>,
-    #[serde(rename = "src-dir")]
+    #[serde(rename = "src_dir")]
     src_dir: Option<PathBuf>,
 }