refactor(core): remove RuntimeOptions::extensions_with_js (#18099)

This commit removes "deno_core::RuntimeOptions::extensions_with_js".

Now it's embedders' responsibility to properly register extensions
that will not contains JavaScript sources when running from an existing
snapshot.

Prerequisite for https://github.com/denoland/deno/pull/18080

bench_util/js_runtime.rs

@@ -9,7 +9,7 @@ use crate::profiling::is_profiling;
 
 pub fn create_js_runtime(setup: impl FnOnce() -> Vec<Extension>) -> JsRuntime {
   JsRuntime::new(RuntimeOptions {
-    extensions_with_js: setup(),
+    extensions: setup(),
     module_loader: Some(
       std::rc::Rc::new(deno_core::ExtModuleLoader::default()),
     ),

cli/build.rs

@@ -276,8 +276,7 @@ mod ts {
       cargo_manifest_dir: env!("CARGO_MANIFEST_DIR"),
       snapshot_path,
       startup_snapshot: None,
-      extensions: vec![],
-      extensions_with_js: vec![tsc_extension],
+      extensions: vec![tsc_extension],
 
       // NOTE(bartlomieju): Compressing the TSC snapshot in debug build took
       // ~45s on M1 MacBook Pro; without compression it took ~1s.
@@ -321,7 +320,7 @@ mod ts {
 }
 
 fn create_cli_snapshot(snapshot_path: PathBuf) {
-  let extensions: Vec<Extension> = vec![
+  let mut extensions: Vec<Extension> = vec![
     deno_webidl::init(),
     deno_console::init(),
     deno_url::init_ops(),
@@ -364,20 +363,21 @@ fn create_cli_snapshot(snapshot_path: PathBuf) {
       std::path::PathBuf::from(deno_runtime::js::PATH_FOR_99_MAIN_JS),
     ),
   });
-  let extensions_with_js = vec![Extension::builder("cli")
-    // FIXME(bartlomieju): information about which extensions were
-    // already snapshotted is not preserved in the snapshot. This should be
-    // fixed, so we can reliably depend on that information.
-    // .dependencies(vec!["runtime"])
-    .esm(esm_files)
-    .build()];
+  extensions.push(
+    Extension::builder("cli")
+      // FIXME(bartlomieju): information about which extensions were
+      // already snapshotted is not preserved in the snapshot. This should be
+      // fixed, so we can reliably depend on that information.
+      // .dependencies(vec!["runtime"])
+      .esm(esm_files)
+      .build(),
+  );
 
   create_snapshot(CreateSnapshotOptions {
     cargo_manifest_dir: env!("CARGO_MANIFEST_DIR"),
     snapshot_path,
     startup_snapshot: Some(deno_runtime::js::deno_isolate_init()),
     extensions,
-    extensions_with_js,
     compression_cb: Some(Box::new(|vec, snapshot_slice| {
       lzzzz::lz4_hc::compress_to_vec(
         snapshot_slice,

cli/standalone.rs

@@ -278,7 +278,6 @@ pub async fn run(
       inspect: ps.options.is_inspecting(),
     },
     extensions: ops::cli_exts(ps),
-    extensions_with_js: vec![],
     startup_snapshot: Some(crate::js::deno_isolate_init()),
     unsafely_ignore_certificate_errors: metadata
       .unsafely_ignore_certificate_errors,

cli/worker.rs

@@ -524,7 +524,6 @@ async fn create_main_worker_internal(
       inspect: ps.options.is_inspecting(),
     },
     extensions,
-    extensions_with_js: vec![],
     startup_snapshot: Some(crate::js::deno_isolate_init()),
     unsafely_ignore_certificate_errors: ps
       .options
@@ -758,7 +757,6 @@ mod tests {
         inspect: false,
       },
       extensions: vec![],
-      extensions_with_js: vec![],
       startup_snapshot: Some(crate::js::deno_isolate_init()),
       unsafely_ignore_certificate_errors: None,
       root_cert_store: None,

core/extensions.rs

@@ -104,7 +104,7 @@ impl Extension {
   /// Check if dependencies have been loaded, and errors if either:
   /// - The extension is depending on itself or an extension with the same name.
   /// - A dependency hasn't been loaded yet.
-  pub fn check_dependencies(&self, previous_exts: &[&mut Extension]) {
+  pub fn check_dependencies(&self, previous_exts: &[Extension]) {
     if let Some(deps) = self.deps {
       'dep_loop: for dep in deps {
         if dep == &self.name {
@@ -124,18 +124,12 @@ impl Extension {
 
   /// returns JS source code to be loaded into the isolate (either at snapshotting,
   /// or at startup).  as a vector of a tuple of the file name, and the source code.
-  pub fn get_js_sources(&self) -> &[ExtensionFileSource] {
-    match &self.js_files {
-      Some(files) => files,
-      None => &[],
-    }
+  pub fn get_js_sources(&self) -> Option<&Vec<ExtensionFileSource>> {
+    self.js_files.as_ref()
   }
 
-  pub fn get_esm_sources(&self) -> &[ExtensionFileSource] {
-    match &self.esm_files {
-      Some(files) => files,
-      None => &[],
-    }
+  pub fn get_esm_sources(&self) -> Option<&Vec<ExtensionFileSource>> {
+    self.esm_files.as_ref()
   }
 
   pub fn get_esm_entry_point(&self) -> Option<&'static str> {

core/ops_builtin.rs

@@ -1,3 +1,4 @@
+use crate::ExtensionBuilder;
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 use crate::error::format_file_name;
 use crate::error::type_error;
@@ -18,38 +19,50 @@ use std::io::stdout;
 use std::io::Write;
 use std::rc::Rc;
 
-pub(crate) fn init_builtins() -> Extension {
+fn ext() -> ExtensionBuilder {
   Extension::builder("core")
+}
+
+fn ops(ext: &mut ExtensionBuilder) -> &mut ExtensionBuilder {
+  let mut ops = vec![
+    op_close::decl(),
+    op_try_close::decl(),
+    op_print::decl(),
+    op_resources::decl(),
+    op_wasm_streaming_feed::decl(),
+    op_wasm_streaming_set_url::decl(),
+    op_void_sync::decl(),
+    op_void_async::decl(),
+    op_add::decl(),
+    // // TODO(@AaronO): track IO metrics for builtin streams
+    op_read::decl(),
+    op_read_all::decl(),
+    op_write::decl(),
+    op_write_all::decl(),
+    op_shutdown::decl(),
+    op_metrics::decl(),
+    op_format_file_name::decl(),
+    op_is_proxy::decl(),
+    op_str_byte_length::decl(),
+  ];
+  ops.extend(crate::ops_builtin_v8::init_builtins_v8());
+  ext.ops(ops)
+}
+
+pub(crate) fn init_builtin_ops_and_esm() -> Extension {
+  ops(&mut ext())
     .js(include_js_files!(
       "00_primordials.js",
       "01_core.js",
       "02_error.js",
     ))
-    .ops(vec![
-      op_close::decl(),
-      op_try_close::decl(),
-      op_print::decl(),
-      op_resources::decl(),
-      op_wasm_streaming_feed::decl(),
-      op_wasm_streaming_set_url::decl(),
-      op_void_sync::decl(),
-      op_void_async::decl(),
-      op_add::decl(),
-      // // TODO(@AaronO): track IO metrics for builtin streams
-      op_read::decl(),
-      op_read_all::decl(),
-      op_write::decl(),
-      op_write_all::decl(),
-      op_shutdown::decl(),
-      op_metrics::decl(),
-      op_format_file_name::decl(),
-      op_is_proxy::decl(),
-      op_str_byte_length::decl(),
-    ])
-    .ops(crate::ops_builtin_v8::init_builtins_v8())
     .build()
 }
 
+pub(crate) fn init_builtin_ops() -> Extension {
+  ops(&mut ext()).build()
+}
+
 /// Return map of resources with id as key
 /// and string representation as value.
 #[op]

core/runtime.rs

@@ -89,7 +89,6 @@ pub struct JsRuntime {
   snapshot_options: SnapshotOptions,
   allocations: IsolateAllocations,
   extensions: Vec<Extension>,
-  extensions_with_js: Vec<Extension>,
   event_loop_middlewares: Vec<Box<OpEventLoopFn>>,
   // Marks if this is considered the top-level runtime. Used only be inspector.
   is_main: bool,
@@ -253,20 +252,12 @@ pub struct RuntimeOptions {
 
   /// JsRuntime extensions, not to be confused with ES modules.
   /// Only ops registered by extensions will be initialized. If you need
-  /// to execute JS code from extensions, use `extensions_with_js` options
-  /// instead.
-  pub extensions: Vec<Extension>,
-
-  /// JsRuntime extensions, not to be confused with ES modules.
-  /// Ops registered by extensions will be initialized and JS code will be
-  /// executed. If you don't need to execute JS code from extensions, use
-  /// `extensions` option instead.
+  /// to execute JS code from extensions, pass source files in `js` or `esm`
+  /// option on `ExtensionBuilder`.
   ///
-  /// This is useful when creating snapshots, in such case you would pass
-  /// extensions using `extensions_with_js`, later when creating a runtime
-  /// from the snapshot, you would pass these extensions using `extensions`
-  /// option.
-  pub extensions_with_js: Vec<Extension>,
+  /// If you are creating a runtime from a snapshot take care not to include
+  /// JavaScript sources in the extensions.
+  pub extensions: Vec<Extension>,
 
   /// V8 snapshot that should be loaded on startup.
   pub startup_snapshot: Option<Snapshot>,
@@ -373,18 +364,15 @@ impl JsRuntime {
     let has_startup_snapshot = options.startup_snapshot.is_some();
     if !has_startup_snapshot {
       options
-        .extensions_with_js
-        .insert(0, crate::ops_builtin::init_builtins());
+        .extensions
+        .insert(0, crate::ops_builtin::init_builtin_ops_and_esm());
     } else {
       options
         .extensions
-        .insert(0, crate::ops_builtin::init_builtins());
+        .insert(0, crate::ops_builtin::init_builtin_ops());
     }
 
-    let ops = Self::collect_ops(
-      &mut options.extensions,
-      &mut options.extensions_with_js,
-    );
+    let ops = Self::collect_ops(&mut options.extensions);
     let mut op_state = OpState::new(ops.len());
 
     if let Some(get_error_class_fn) = options.get_error_class_fn {
@@ -624,9 +612,12 @@ impl JsRuntime {
 
     let loader = if snapshot_options != SnapshotOptions::Load {
       let esm_sources = options
-        .extensions_with_js
+        .extensions
         .iter()
-        .flat_map(|ext| ext.get_esm_sources().to_owned())
+        .flat_map(|ext| match ext.get_esm_sources() {
+          Some(s) => s.to_owned(),
+          None => vec![],
+        })
         .collect::<Vec<ExtensionFileSource>>();
 
       #[cfg(feature = "include_js_files_for_snapshotting")]
@@ -689,7 +680,6 @@ impl JsRuntime {
       allocations: IsolateAllocations::default(),
       event_loop_middlewares: Vec::with_capacity(options.extensions.len()),
       extensions: options.extensions,
-      extensions_with_js: options.extensions_with_js,
       state: state_rc,
       module_map: Some(module_map_rc),
       is_main: options.is_main,
@@ -752,7 +742,7 @@ impl JsRuntime {
 
   /// Creates a new realm (V8 context) in this JS execution context,
   /// pre-initialized with all of the extensions that were passed in
-  /// [`RuntimeOptions::extensions_with_js`] when the [`JsRuntime`] was
+  /// [`RuntimeOptions::extensions`] when the [`JsRuntime`] was
   /// constructed.
   pub fn create_realm(&mut self) -> Result<JsRealm, Error> {
     let realm = {
@@ -868,50 +858,45 @@ impl JsRuntime {
     }
 
     // Take extensions to avoid double-borrow
-    let extensions = std::mem::take(&mut self.extensions_with_js);
+    let extensions = std::mem::take(&mut self.extensions);
     for ext in &extensions {
       {
-        let esm_files = ext.get_esm_sources();
-        if let Some(entry_point) = ext.get_esm_entry_point() {
-          let file_source = esm_files
-            .iter()
-            .find(|file| file.specifier == entry_point)
-            .unwrap();
-          load_and_evaluate_module(self, file_source)?;
-        } else {
-          for file_source in esm_files {
+        if let Some(esm_files) = ext.get_esm_sources() {
+          if let Some(entry_point) = ext.get_esm_entry_point() {
+            let file_source = esm_files
+              .iter()
+              .find(|file| file.specifier == entry_point)
+              .unwrap();
             load_and_evaluate_module(self, file_source)?;
+          } else {
+            for file_source in esm_files {
+              load_and_evaluate_module(self, file_source)?;
+            }
           }
         }
       }
 
       {
-        let js_files = ext.get_js_sources();
-        for file_source in js_files {
-          // TODO(@AaronO): use JsRuntime::execute_static() here to move src off heap
-          realm.execute_script(
-            self.v8_isolate(),
-            &file_source.specifier,
-            &file_source.code.load()?,
-          )?;
+        if let Some(js_files) = ext.get_js_sources() {
+          for file_source in js_files {
+            // TODO(@AaronO): use JsRuntime::execute_static() here to move src off heap
+            realm.execute_script(
+              self.v8_isolate(),
+              &file_source.specifier,
+              &file_source.code.load()?,
+            )?;
+          }
         }
       }
     }
     // Restore extensions
-    self.extensions_with_js = extensions;
+    self.extensions = extensions;
 
     Ok(())
   }
 
   /// Collects ops from extensions & applies middleware
-  fn collect_ops(
-    extensions: &mut [Extension],
-    extensions_with_js: &mut [Extension],
-  ) -> Vec<OpDecl> {
-    let mut exts = vec![];
-    exts.extend(extensions);
-    exts.extend(extensions_with_js);
-
+  fn collect_ops(exts: &mut [Extension]) -> Vec<OpDecl> {
     for (ext, previous_exts) in
       exts.iter().enumerate().map(|(i, ext)| (ext, &exts[..i]))
     {
@@ -970,24 +955,6 @@ impl JsRuntime {
       // Restore extensions
       self.extensions = extensions;
     }
-    {
-      let mut extensions: Vec<Extension> =
-        std::mem::take(&mut self.extensions_with_js);
-
-      // Setup state
-      for e in extensions.iter_mut() {
-        // ops are already registered during in bindings::initialize_context();
-        e.init_state(&mut op_state.borrow_mut());
-
-        // Setup event-loop middleware
-        if let Some(middleware) = e.init_event_loop_middleware() {
-          self.event_loop_middlewares.push(middleware);
-        }
-      }
-
-      // Restore extensions
-      self.extensions_with_js = extensions;
-    }
     Ok(())
   }
 

core/snapshot_util.rs

@@ -17,7 +17,6 @@ pub struct CreateSnapshotOptions {
   pub snapshot_path: PathBuf,
   pub startup_snapshot: Option<Snapshot>,
   pub extensions: Vec<Extension>,
-  pub extensions_with_js: Vec<Extension>,
   pub compression_cb: Option<Box<CompressionCb>>,
   pub snapshot_module_load_cb: Option<ExtModuleLoaderCb>,
 }
@@ -29,7 +28,6 @@ pub fn create_snapshot(create_snapshot_options: CreateSnapshotOptions) {
     will_snapshot: true,
     startup_snapshot: create_snapshot_options.startup_snapshot,
     extensions: create_snapshot_options.extensions,
-    extensions_with_js: create_snapshot_options.extensions_with_js,
     snapshot_module_load_cb: create_snapshot_options.snapshot_module_load_cb,
     ..Default::default()
   });

ext/fetch/lib.rs

@@ -149,17 +149,7 @@ pub fn init_ops<FP>(options: Options) -> Extension
 where
   FP: FetchPermissions + 'static,
 {
-  ops::<FP>(&mut ext(), options)
-    .esm(include_js_files!(
-      "20_headers.js",
-      "21_formdata.js",
-      "22_body.js",
-      "22_http_client.js",
-      "23_request.js",
-      "23_response.js",
-      "26_fetch.js",
-    ))
-    .build()
+  ops::<FP>(&mut ext(), options).build()
 }
 
 pub type CancelableResponseFuture =

ext/ffi/lib.rs

@@ -169,7 +169,5 @@ pub fn init_ops_and_esm<P: FfiPermissions + 'static>(
 }
 
 pub fn init_ops<P: FfiPermissions + 'static>(unstable: bool) -> Extension {
-  ops::<P>(&mut ext(), unstable)
-    .esm(include_js_files!("00_ffi.js",))
-    .build()
+  ops::<P>(&mut ext(), unstable).build()
 }

ext/fs/lib.rs

@@ -202,9 +202,7 @@ pub fn init_ops_and_esm<P: FsPermissions + 'static>(
 }
 
 pub fn init_ops<P: FsPermissions + 'static>(unstable: bool) -> Extension {
-  ops::<P>(&mut ext(), unstable)
-    .esm(include_js_files!("30_fs.js",))
-    .build()
+  ops::<P>(&mut ext(), unstable).build()
 }
 
 fn default_err_mapper(err: Error, desc: String) -> Error {

runtime/build.rs

@@ -250,7 +250,7 @@ mod startup_snapshot {
     ))
     .build();
 
-    let mut extensions_with_js: Vec<Extension> = vec![
+    let mut extensions: Vec<Extension> = vec![
       deno_webidl::init_esm(),
       deno_console::init_esm(),
       deno_url::init_ops_and_esm(),
@@ -291,15 +291,14 @@ mod startup_snapshot {
     ];
 
     if let Some(additional_extension) = maybe_additional_extension {
-      extensions_with_js.push(additional_extension);
+      extensions.push(additional_extension);
     }
 
     create_snapshot(CreateSnapshotOptions {
       cargo_manifest_dir: env!("CARGO_MANIFEST_DIR"),
       snapshot_path,
       startup_snapshot: None,
-      extensions: vec![],
-      extensions_with_js,
+      extensions,
       compression_cb: Some(Box::new(|vec, snapshot_slice| {
         lzzzz::lz4_hc::compress_to_vec(
           snapshot_slice,

runtime/examples/hello_runtime.rs

@@ -43,7 +43,6 @@ async fn main() -> Result<(), AnyError> {
       inspect: false,
     },
     extensions: vec![],
-    extensions_with_js: vec![],
     startup_snapshot: None,
     unsafely_ignore_certificate_errors: None,
     root_cert_store: None,

runtime/worker.rs

@@ -72,21 +72,11 @@ pub struct WorkerOptions {
   pub bootstrap: BootstrapOptions,
 
   /// JsRuntime extensions, not to be confused with ES modules.
-  /// Only ops registered by extensions will be initialized. If you need
-  /// to execute JS code from extensions, use `extensions_with_js` options
-  /// instead.
-  pub extensions: Vec<Extension>,
-
-  /// JsRuntime extensions, not to be confused with ES modules.
-  /// Ops registered by extensions will be initialized and JS code will be
-  /// executed. If you don't need to execute JS code from extensions, use
-  /// `extensions` option instead.
   ///
-  /// This is useful when creating snapshots, in such case you would pass
-  /// extensions using `extensions_with_js`, later when creating a runtime
-  /// from the snapshot, you would pass these extensions using `extensions`
-  /// option.
-  pub extensions_with_js: Vec<Extension>,
+  /// Extensions register "ops" and JavaScript sources provided in `js` or `esm`
+  /// configuration. If you are using a snapshot, then extensions shouldn't
+  /// provide JavaScript sources that were already snapshotted.
+  pub extensions: Vec<Extension>,
 
   /// V8 snapshot that should be loaded on startup.
   pub startup_snapshot: Option<Snapshot>,
@@ -174,7 +164,6 @@ impl Default for WorkerOptions {
       npm_resolver: Default::default(),
       blob_store: Default::default(),
       extensions: Default::default(),
-      extensions_with_js: Default::default(),
       startup_snapshot: Default::default(),
       bootstrap: Default::default(),
       stdio: Default::default(),
@@ -299,7 +288,6 @@ impl MainWorker {
       shared_array_buffer_store: options.shared_array_buffer_store.clone(),
       compiled_wasm_module_store: options.compiled_wasm_module_store.clone(),
       extensions,
-      extensions_with_js: options.extensions_with_js,
       inspector: options.maybe_inspector_server.is_some(),
       is_main: true,
       leak_isolate: options.leak_isolate,