Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Deprecate "import assertions" with a warning #24743

Merged
merged 15 commits into from
Aug 19, 2024
13 changes: 13 additions & 0 deletions cli/cache/code_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ impl CodeCache {
data,
));
}

pub fn remove_code_cache(&self, specifier: &str) {
Self::ensure_ok(self.inner.remove_code_cache(specifier))
}
}

impl code_cache::CodeCache for CodeCache {
Expand Down Expand Up @@ -158,6 +162,15 @@ impl CodeCacheInner {
self.conn.execute(sql, params)?;
Ok(())
}

pub fn remove_code_cache(&self, specifier: &str) -> Result<(), AnyError> {
let sql = "
DELETE FROM codecache
WHERE specifier=$1;";
let params = params![specifier];
self.conn.execute(sql, params)?;
Ok(())
}
}

fn serialize_code_cache_type(
Expand Down
5 changes: 5 additions & 0 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,11 +438,15 @@ fn resolve_flags_and_init(
DenoSubcommand::Lsp => vec!["--max-old-space-size=3072".to_string()],
_ => {
if *DENO_FUTURE {
// TODO(bartlomieju): I think this can be removed as it's handled by `deno_core`
// and its settings.
// deno_ast removes TypeScript `assert` keywords, so this flag only affects JavaScript
// TODO(petamoriken): Need to check TypeScript `assert` keywords in deno_ast
vec!["--no-harmony-import-assertions".to_string()]
} else {
vec![
// TODO(bartlomieju): I think this can be removed as it's handled by `deno_core`
// and its settings.
// If we're still in v1.X version we want to support import assertions.
// V8 12.6 unshipped the support by default, so force it by passing a
// flag.
Expand All @@ -455,6 +459,7 @@ fn resolve_flags_and_init(
};

init_v8_flags(&default_v8_flags, &flags.v8_flags, get_v8_flags_from_env());
// TODO(bartlomieju): remove last argument in Deno 2.
deno_core::JsRuntime::init_platform(None, !*DENO_FUTURE);
util::logger::init(flags.log_level);

Expand Down
28 changes: 28 additions & 0 deletions cli/module_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use std::borrow::Cow;
use std::cell::RefCell;
use std::collections::HashSet;
use std::path::PathBuf;
use std::pin::Pin;
use std::rc::Rc;
Expand Down Expand Up @@ -44,6 +45,7 @@ use deno_core::error::generic_error;
use deno_core::error::AnyError;
use deno_core::futures::future::FutureExt;
use deno_core::futures::Future;
use deno_core::parking_lot::Mutex;
use deno_core::resolve_url;
use deno_core::ModuleCodeString;
use deno_core::ModuleLoader;
Expand Down Expand Up @@ -291,6 +293,7 @@ impl CliModuleLoaderFactory {
emitter: self.shared.emitter.clone(),
parsed_source_cache: self.shared.parsed_source_cache.clone(),
shared: self.shared.clone(),
prevent_v8_code_cache: Default::default(),
})));
ModuleLoaderAndSourceMapGetter {
module_loader: loader,
Expand Down Expand Up @@ -342,6 +345,10 @@ struct CliModuleLoaderInner<TGraphContainer: ModuleGraphContainer> {
emitter: Arc<Emitter>,
parsed_source_cache: Arc<ParsedSourceCache>,
graph_container: TGraphContainer,
// NOTE(bartlomieju): this is temporary, for deprecated import assertions.
// Should be removed in Deno 2.
// Modules stored here should not be V8 code-cached.
prevent_v8_code_cache: Arc<Mutex<HashSet<String>>>,
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be an in Arc? Maybe use DashSet instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's a big deal? This code is gonna be gone in a few days anyway.

}

impl<TGraphContainer: ModuleGraphContainer>
Expand Down Expand Up @@ -827,6 +834,14 @@ impl<TGraphContainer: ModuleGraphContainer> ModuleLoader
code_cache: &[u8],
) -> Pin<Box<dyn Future<Output = ()>>> {
if let Some(cache) = self.0.shared.code_cache.as_ref() {
if self
.0
.prevent_v8_code_cache
.lock()
.contains(specifier.as_str())
{
return std::future::ready(()).boxed_local();
}
// This log line is also used by tests.
log::debug!(
"Updating V8 code cache for ES module: {specifier}, [{source_hash:?}]"
Expand All @@ -841,6 +856,19 @@ impl<TGraphContainer: ModuleGraphContainer> ModuleLoader
std::future::ready(()).boxed_local()
}

fn purge_and_prevent_code_cache(&self, specifier: &str) {
if let Some(cache) = self.0.shared.code_cache.as_ref() {
// This log line is also used by tests.
log::debug!("Remove V8 code cache for ES module: {specifier}");
cache.remove_code_cache(specifier);
self
.0
.prevent_v8_code_cache
.lock()
.insert(specifier.to_string());
}
}

fn get_source_map(&self, file_name: &str) -> Option<Vec<u8>> {
let specifier = resolve_url(file_name).ok()?;
match specifier.scheme() {
Expand Down
1 change: 1 addition & 0 deletions cli/standalone/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,7 @@ pub async fn run(

// Initialize v8 once from the main thread.
v8_set_flags(construct_v8_flags(&[], &metadata.v8_flags, vec![]));
// TODO(bartlomieju): remove last argument in Deno 2.
deno_core::JsRuntime::init_platform(None, true);

let mut worker = worker_factory
Expand Down
23 changes: 23 additions & 0 deletions runtime/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,26 @@ pub fn maybe_transpile_source(

Ok((source_text.into(), maybe_source_map))
}

pub fn import_assertion_callback(
args: deno_core::ImportAssertionsSupportCustomCallbackArgs,
) {
let mut msg = deno_terminal::colors::yellow("⚠️ Import assertions are deprecated. Use `with` keyword, instead of 'assert' keyword.").to_string();
if let Some(specifier) = args.maybe_specifier {
if let Some(source_line) = args.maybe_source_line {
msg.push_str("\n\n");
msg.push_str(&source_line);
msg.push_str("\n\n");
}
msg.push_str(&format!(
" at {}:{}:{}\n",
specifier,
args.maybe_line_number.unwrap(),
args.column_number
));
#[allow(clippy::print_stderr)]
{
eprintln!("{}", msg);
}
}
}
8 changes: 8 additions & 0 deletions runtime/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,13 @@ impl WebWorker {
options.bootstrap.enable_op_summary_metrics,
options.strace_ops,
);
let import_assertions_support = if options.bootstrap.future {
deno_core::ImportAssertionsSupport::Error
} else {
deno_core::ImportAssertionsSupport::CustomCallback(Box::new(
crate::shared::import_assertion_callback,
))
};

let mut js_runtime = JsRuntime::new(RuntimeOptions {
module_loader: Some(options.module_loader.clone()),
Expand All @@ -558,6 +565,7 @@ impl WebWorker {
validate_import_attributes_cb: Some(Box::new(
validate_import_attributes_callback,
)),
import_assertions_support,
..Default::default()
});

Expand Down
20 changes: 9 additions & 11 deletions runtime/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,14 @@ impl MainWorker {
}
});

let import_assertions_support = if options.bootstrap.future {
deno_core::ImportAssertionsSupport::Error
} else {
deno_core::ImportAssertionsSupport::CustomCallback(Box::new(
crate::shared::import_assertion_callback,
))
};

let mut js_runtime = JsRuntime::new(RuntimeOptions {
module_loader: Some(options.module_loader.clone()),
startup_snapshot: options.startup_snapshot,
Expand All @@ -501,6 +509,7 @@ impl MainWorker {
validate_import_attributes_cb: Some(Box::new(
validate_import_attributes_callback,
)),
import_assertions_support,
eval_context_code_cache_cbs: options.v8_code_cache.map(|cache| {
let cache_clone = cache.clone();
(
Expand Down Expand Up @@ -544,17 +553,6 @@ impl MainWorker {
if let Some(op_summary_metrics) = op_summary_metrics {
js_runtime.op_state().borrow_mut().put(op_summary_metrics);
}
extern "C" fn message_handler(
_msg: v8::Local<v8::Message>,
_exception: v8::Local<v8::Value>,
) {
// TODO(@littledivy): Propogate message to users.
}

// Register message listener
js_runtime
.v8_isolate()
.add_message_listener(message_handler);

if let Some(server) = options.maybe_inspector_server.clone() {
server.register_inspector(
Expand Down
9 changes: 9 additions & 0 deletions tests/specs/future/import_assertions/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
{
"steps": [
{
"args": "run main.js",
"output": "error.out",
"exitCode": 1,
"envs": {
"DENO_FUTURE": "1"
}
},
// Running the same multiple times, should warn each time.
{
"args": "run main.js",
"output": "error.out",
Expand Down
6 changes: 6 additions & 0 deletions tests/specs/future/import_assertions/success.out
Original file line number Diff line number Diff line change
@@ -1 +1,7 @@
⚠️ Import assertions are deprecated. Use `with` keyword, instead of 'assert' keyword.

import foo from "./main.json" assert { type: "json" };

at [WILDCARD]import_assertions/main.js:1:30

{ foo: "foo" }