From 050144e0c6fdfdda9d21a678eaa2459caeac7a3d Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Sun, 22 Sep 2024 09:40:49 +0200 Subject: [PATCH] Turbopack: improve edge runtime checker (#70184) Closes PACK-3254 1. Previously, guards were never removed, so a single if statement containing `if(clearImmediate)` would mean that it's valid in the whole file to use `clearImmediate` anywhere. Now it's only valid inside the bodies of conditional. I haven't seen any additional lints caused by this. 2. `if(process.env.NEXT_RUNTIME === 'edge')` guards are now respected There are new test cases for each of these. One new warning from this is the following from `react/cjs/react.development.js` where the unrelated `typeof` doesn't silence the error for `new MessageChannel()` anymore ``` function enqueueTask(task) { if (null === enqueueTaskImpl) try { var requireString = ("require" + Math.random()).slice(0, 7); enqueueTaskImpl = (module && module[requireString]).call( module, "timers" ).setImmediate; } catch (_err) { enqueueTaskImpl = function (callback) { !1 === didWarnAboutMessageChannel && ((didWarnAboutMessageChannel = !0), "undefined" === typeof MessageChannel && console.error( "This browser does not have a MessageChannel implementation, so enqueuing tasks via await act(async () => ...) will fail. Please file an issue at https://github.com/facebook/react/issues if you encounter this warning." )); var channel = new MessageChannel(); /// <--------- channel.port1.onmessage = callback; channel.port2.postMessage(void 0); }; } return enqueueTaskImpl(task); } ``` --- .../src/transforms/warn_for_edge_runtime.rs | 86 ++++++++++++++----- .../guarded-runtime-process/input.js | 5 ++ .../guarded-runtime-process/output.js | 5 ++ .../edge-assert/mixed-process-api/input.js | 7 ++ .../edge-assert/mixed-process-api/output.js | 5 ++ .../mixed-process-api/output.stderr | 7 ++ 6 files changed, 95 insertions(+), 20 deletions(-) create mode 100644 crates/next-custom-transforms/tests/fixture/edge-assert/guarded-runtime-process/input.js create mode 100644 crates/next-custom-transforms/tests/fixture/edge-assert/guarded-runtime-process/output.js create mode 100644 crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/input.js create mode 100644 crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/output.js create mode 100644 crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/output.stderr diff --git a/crates/next-custom-transforms/src/transforms/warn_for_edge_runtime.rs b/crates/next-custom-transforms/src/transforms/warn_for_edge_runtime.rs index 558597045444d..8b38ffe44f652 100644 --- a/crates/next-custom-transforms/src/transforms/warn_for_edge_runtime.rs +++ b/crates/next-custom-transforms/src/transforms/warn_for_edge_runtime.rs @@ -1,6 +1,5 @@ use std::sync::Arc; -use rustc_hash::FxHashSet; use swc_core::{ atoms::Atom, common::{errors::HANDLER, SourceMap, Span}, @@ -27,20 +26,31 @@ pub fn warn_for_edge_runtime( should_add_guards: false, guarded_symbols: Default::default(), guarded_process_props: Default::default(), + guarded_runtime: false, is_production, } } +/// This is a very simple visitor that currently only checks if a condition (be it an if-statement +/// or ternary expression) contains a reference to disallowed globals/etc. +/// It does not know the difference between +/// ```js +/// if(typeof clearImmediate === "function") clearImmediate(); +/// ``` +/// and +/// ```js +/// if(typeof clearImmediate !== "function") clearImmediate(); +/// ``` struct WarnForEdgeRuntime { cm: Arc, ctx: ExprCtx, should_error_for_node_apis: bool, should_add_guards: bool, - /// We don't drop guards because a user may write a code like - /// `if(typeof clearImmediate !== "function") clearImmediate();` - guarded_symbols: FxHashSet, - guarded_process_props: FxHashSet, + guarded_symbols: Vec, + guarded_process_props: Vec, + // for process.env.NEXT_RUNTIME + guarded_runtime: bool, is_production: bool, } @@ -136,6 +146,10 @@ const NODEJS_MODULE_NAMES: &[&str] = &[ impl WarnForEdgeRuntime { fn warn_if_nodejs_module(&self, span: Span, module_specifier: &str) -> Option<()> { + if self.guarded_runtime { + return None; + } + // Node.js modules can be loaded with `node:` prefix or directly if module_specifier.starts_with("node:") || NODEJS_MODULE_NAMES.contains(&module_specifier) { @@ -157,10 +171,11 @@ Learn More: https://nextjs.org/docs/messages/node-module-in-edge-runtime", } fn emit_unsupported_api_error(&self, span: Span, api_name: &str) -> Option<()> { - if self - .guarded_symbols - .iter() - .any(|guarded| guarded == api_name) + if self.guarded_runtime + || self + .guarded_symbols + .iter() + .any(|guarded| guarded == api_name) { return None; } @@ -193,7 +208,7 @@ Learn more: https://nextjs.org/docs/api-reference/edge-runtime", if !self.is_in_middleware_layer() || prop.sym == "env" { return; } - if self.guarded_process_props.contains(&prop.sym) { + if self.guarded_runtime || self.guarded_process_props.contains(&prop.sym) { return; } @@ -214,12 +229,21 @@ Learn more: https://nextjs.org/docs/api-reference/edge-runtime", match test { Expr::Ident(ident) => { - self.guarded_symbols.insert(ident.sym.clone()); + self.guarded_symbols.push(ident.sym.clone()); } Expr::Member(member) => { + if member.prop.is_ident_with("NEXT_RUNTIME") { + if let Expr::Member(obj_member) = &*member.obj { + if obj_member.obj.is_global_ref_to(&self.ctx, "process") + && obj_member.prop.is_ident_with("env") + { + self.guarded_runtime = true; + } + } + } if member.obj.is_global_ref_to(&self.ctx, "process") { if let MemberProp::Ident(prop) = &member.prop { - self.guarded_process_props.insert(prop.sym.clone()); + self.guarded_process_props.push(prop.sym.clone()); } } } @@ -247,6 +271,17 @@ Learn more: https://nextjs.org/docs/api-reference/edge-runtime", }); } } + + fn with_new_scope(&mut self, f: impl FnOnce(&mut Self)) { + let old_guarded_symbols_len = self.guarded_symbols.len(); + let old_guarded_process_props_len = self.guarded_symbols.len(); + let old_guarded_runtime = self.guarded_runtime; + f(self); + self.guarded_symbols.truncate(old_guarded_symbols_len); + self.guarded_process_props + .truncate(old_guarded_process_props_len); + self.guarded_runtime = old_guarded_runtime; + } } impl Visit for WarnForEdgeRuntime { @@ -263,8 +298,15 @@ impl Visit for WarnForEdgeRuntime { fn visit_bin_expr(&mut self, node: &BinExpr) { match node.op { op!("&&") | op!("||") | op!("??") => { - self.add_guards(&node.left); - node.right.visit_with(self); + self.with_new_scope(move |this| { + this.add_guards(&node.left); + node.right.visit_with(this); + }); + } + op!("==") | op!("===") => { + self.add_guard_for_test(&node.left); + self.add_guard_for_test(&node.right); + node.visit_children_with(self); } _ => { node.visit_children_with(self); @@ -272,10 +314,12 @@ impl Visit for WarnForEdgeRuntime { } } fn visit_cond_expr(&mut self, node: &CondExpr) { - self.add_guards(&node.test); + self.with_new_scope(move |this| { + this.add_guards(&node.test); - node.cons.visit_with(self); - node.alt.visit_with(self); + node.cons.visit_with(this); + node.alt.visit_with(this); + }); } fn visit_expr(&mut self, n: &Expr) { @@ -299,10 +343,12 @@ impl Visit for WarnForEdgeRuntime { } fn visit_if_stmt(&mut self, node: &IfStmt) { - self.add_guards(&node.test); + self.with_new_scope(move |this| { + this.add_guards(&node.test); - node.cons.visit_with(self); - node.alt.visit_with(self); + node.cons.visit_with(this); + node.alt.visit_with(this); + }); } fn visit_import_decl(&mut self, n: &ImportDecl) { diff --git a/crates/next-custom-transforms/tests/fixture/edge-assert/guarded-runtime-process/input.js b/crates/next-custom-transforms/tests/fixture/edge-assert/guarded-runtime-process/input.js new file mode 100644 index 0000000000000..e3335ea57cf76 --- /dev/null +++ b/crates/next-custom-transforms/tests/fixture/edge-assert/guarded-runtime-process/input.js @@ -0,0 +1,5 @@ +if (process.env.NEXT_RUNTIME === 'edge') { + setTimeout(cb, 0) +} else { + setImmediate(cb) +} diff --git a/crates/next-custom-transforms/tests/fixture/edge-assert/guarded-runtime-process/output.js b/crates/next-custom-transforms/tests/fixture/edge-assert/guarded-runtime-process/output.js new file mode 100644 index 0000000000000..d8402d1c59957 --- /dev/null +++ b/crates/next-custom-transforms/tests/fixture/edge-assert/guarded-runtime-process/output.js @@ -0,0 +1,5 @@ +if (process.env.NEXT_RUNTIME === 'edge') { + setTimeout(cb, 0); +} else { + setImmediate(cb); +} diff --git a/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/input.js b/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/input.js new file mode 100644 index 0000000000000..ce2c67494a4b1 --- /dev/null +++ b/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/input.js @@ -0,0 +1,7 @@ +if (typeof process.loadEnvFile === 'function') { + console.log(process.loadEnvFile()) +} + +typeof process.loadEnvFile === 'function' && process.loadEnvFile() + +console.log(process.loadEnvFile()) diff --git a/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/output.js b/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/output.js new file mode 100644 index 0000000000000..e3cbf9a95256d --- /dev/null +++ b/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/output.js @@ -0,0 +1,5 @@ +if (typeof process.loadEnvFile === 'function') { + console.log(process.loadEnvFile()); +} +typeof process.loadEnvFile === 'function' && process.loadEnvFile(); +console.log(process.loadEnvFile()); diff --git a/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/output.stderr b/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/output.stderr new file mode 100644 index 0000000000000..3c4a5659da4e8 --- /dev/null +++ b/crates/next-custom-transforms/tests/fixture/edge-assert/mixed-process-api/output.stderr @@ -0,0 +1,7 @@ + x A Node.js API is used (process.loadEnvFile at line: 7) which is not supported in the Edge Runtime. + | Learn more: https://nextjs.org/docs/api-reference/edge-runtime + ,-[input.js:7:1] + 6 | + 7 | console.log(process.loadEnvFile()) + : ^^^^^^^^^^^^^^^^^^^ + `----