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(node): stabilize detecting if CJS via "type": "commonjs" in a package.json #26439

Merged
merged 28 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
bcfad0d
fix(node): requiring a .js file in Deno code should be treated as ESM
dsherret Oct 20, 2024
d69a3e9
remove todo
dsherret Oct 20, 2024
ef56f5e
lint
dsherret Oct 20, 2024
3afd732
fix node compat tests probably
dsherret Oct 20, 2024
6953eab
Maybe fix.
dsherret Oct 20, 2024
ea9002a
fix spec failures
dsherret Oct 20, 2024
cf3d987
Merge branch 'main' into fix_require_js_esm
dsherret Oct 21, 2024
57273d9
Merge branch 'main' into fix_require_js_esm
dsherret Nov 8, 2024
ebd12a5
fixes
dsherret Nov 8, 2024
72107f3
stabilize detect-cjs
dsherret Nov 8, 2024
848dc74
Some fixes for detect-cjs
dsherret Nov 8, 2024
9f44576
remove npm main
dsherret Nov 8, 2024
d509fda
Fix.
dsherret Nov 8, 2024
16b3b4a
Starting to separate out cjs tracker, but realizing that graph resolv…
dsherret Nov 11, 2024
3f064b9
Merge branch 'main' into fix_require_js_esm
dsherret Nov 11, 2024
982d8c4
Figuring this out... CjsTracker should not be used in the lsp
dsherret Nov 11, 2024
af5a374
compiling
dsherret Nov 11, 2024
e3134c0
fix tests
dsherret Nov 11, 2024
c8f5141
fix issue with type checking not resolving based on package.json
dsherret Nov 11, 2024
63f6255
switch eval to mts
dsherret Nov 11, 2024
1ff3573
fix mac
dsherret Nov 12, 2024
fa3bd0e
add final lsp test
dsherret Nov 12, 2024
c242e5c
Merge branch 'main' into fix_require_js_esm
dsherret Nov 12, 2024
c84900e
fix
dsherret Nov 12, 2024
fe03ea7
Align require node module kind detection, handle resolving `node` bin…
dsherret Nov 12, 2024
d1184c7
Merge branch 'main' into fix_require_js_esm
dsherret Nov 12, 2024
c4256e9
add comment
dsherret Nov 13, 2024
0014ec6
Merge branch 'main' into fix_require_js_esm
dsherret Nov 13, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions cli/args/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,6 @@ pub struct UnstableConfig {
// TODO(bartlomieju): remove in Deno 2.5
pub legacy_flag_enabled: bool, // --unstable
pub bare_node_builtins: bool,
pub detect_cjs: bool,
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
pub sloppy_imports: bool,
pub features: Vec<String>, // --unstabe-kv --unstable-cron
}
Expand Down Expand Up @@ -5720,7 +5719,6 @@ fn unstable_args_parse(

flags.unstable_config.bare_node_builtins =
matches.get_flag("unstable-bare-node-builtins");
flags.unstable_config.detect_cjs = matches.get_flag("unstable-detect-cjs");
flags.unstable_config.sloppy_imports =
matches.get_flag("unstable-sloppy-imports");

Expand Down
70 changes: 56 additions & 14 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ mod import_map;
mod lockfile;
mod package_json;

use deno_ast::MediaType;
use deno_ast::SourceMapOption;
use deno_config::deno_json::NodeModulesDirMode;
use deno_config::workspace::CreateResolverOptions;
Expand Down Expand Up @@ -34,7 +35,6 @@ use import_map::resolve_import_map_value_from_specifier;
pub use deno_config::deno_json::BenchConfig;
pub use deno_config::deno_json::ConfigFile;
pub use deno_config::deno_json::FmtOptionsConfig;
pub use deno_config::deno_json::JsxImportSourceConfig;
pub use deno_config::deno_json::LintRulesConfig;
pub use deno_config::deno_json::ProseWrap;
pub use deno_config::deno_json::TsConfig;
Expand Down Expand Up @@ -1155,21 +1155,34 @@ impl CliOptions {
self
.main_module_cell
.get_or_init(|| {
let main_module = match &self.flags.subcommand {
Ok(match &self.flags.subcommand {
DenoSubcommand::Compile(compile_flags) => {
resolve_url_or_path(&compile_flags.source_file, self.initial_cwd())?
}
DenoSubcommand::Eval(_) => {
resolve_url_or_path("./$deno$eval.ts", self.initial_cwd())?
resolve_url_or_path("./$deno$eval.mts", self.initial_cwd())?
}
DenoSubcommand::Repl(_) => {
resolve_url_or_path("./$deno$repl.ts", self.initial_cwd())?
resolve_url_or_path("./$deno$repl.mts", self.initial_cwd())?
}
DenoSubcommand::Run(run_flags) => {
if run_flags.is_stdin() {
resolve_url_or_path("./$deno$stdin.ts", self.initial_cwd())?
resolve_url_or_path("./$deno$stdin.mts", self.initial_cwd())?
Copy link
Member Author

Choose a reason for hiding this comment

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

Uses mts now so that these commands continue using esm even if the current directory has a "type": "commonjs"

} else {
resolve_url_or_path(&run_flags.script, self.initial_cwd())?
let url =
resolve_url_or_path(&run_flags.script, self.initial_cwd())?;
if self.is_node_main()
&& url.scheme() == "file"
&& MediaType::from_specifier(&url) == MediaType::Unknown
{
try_resolve_node_binary_main_entrypoint(
&run_flags.script,
self.initial_cwd(),
)?
.unwrap_or(url)
} else {
url
}
}
}
DenoSubcommand::Serve(run_flags) => {
Expand All @@ -1178,9 +1191,7 @@ impl CliOptions {
_ => {
bail!("No main module.")
}
};

Ok(main_module)
})
})
.as_ref()
.map_err(|err| deno_core::anyhow::anyhow!("{}", err))
Expand Down Expand Up @@ -1229,7 +1240,7 @@ impl CliOptions {
// This is triggered via a secret environment variable which is used
// for functionality like child_process.fork. Users should NOT depend
// on this functionality.
pub fn is_npm_main(&self) -> bool {
pub fn is_node_main(&self) -> bool {
NPM_PROCESS_STATE.is_some()
}

Expand Down Expand Up @@ -1607,9 +1618,11 @@ impl CliOptions {
|| self.workspace().has_unstable("bare-node-builtins")
}

pub fn unstable_detect_cjs(&self) -> bool {
self.flags.unstable_config.detect_cjs
|| self.workspace().has_unstable("detect-cjs")
pub fn detect_cjs(&self) -> bool {
// only enabled when there's a package.json in order to not have a
// perf penalty for non-npm Deno projects of searching for the closest
// package.json beside each module
self.workspace().package_jsons().next().is_some() || self.is_node_main()
}

fn byonm_enabled(&self) -> bool {
Expand Down Expand Up @@ -1673,7 +1686,6 @@ impl CliOptions {
"byonm",
"bare-node-builtins",
"fmt-component",
"detect-cjs",
])
.collect();

Expand Down Expand Up @@ -1811,6 +1823,36 @@ fn resolve_node_modules_folder(
Ok(Some(canonicalize_path_maybe_not_exists(&path)?))
}

fn try_resolve_node_binary_main_entrypoint(
specifier: &str,
initial_cwd: &Path,
) -> Result<Option<Url>, AnyError> {
// node allows running files at paths without a `.js` extension
// or at directories with an index.js file
let path = deno_core::normalize_path(initial_cwd.join(specifier));
if path.is_dir() {
let index_file = path.join("index.js");
Ok(if index_file.is_file() {
Some(deno_path_util::url_from_file_path(&index_file)?)
} else {
None
})
} else {
let path = path.with_extension(
path
.extension()
.and_then(|s| s.to_str())
.map(|s| format!("{}.js", s))
.unwrap_or("js".to_string()),
);
if path.is_file() {
Ok(Some(deno_path_util::url_from_file_path(&path)?))
} else {
Ok(None)
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes #26742

The issue here is previously we happened to resolve these kind of entrypoints because it would try running the specifier as a CJS entrypoint with an "unknown" media type.


fn resolve_import_map_specifier(
maybe_import_map_path: Option<&str>,
maybe_config_file: Option<&ConfigFile>,
Expand Down
8 changes: 6 additions & 2 deletions cli/emit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ impl Emitter {
pub async fn load_and_emit_for_hmr(
&self,
specifier: &ModuleSpecifier,
module_kind: deno_ast::ModuleKind,
) -> Result<String, AnyError> {
let media_type = MediaType::from_specifier(specifier);
let source_code = tokio::fs::read_to_string(
Expand All @@ -203,11 +202,16 @@ impl Emitter {
// this statement is probably wrong)
let mut options = self.transpile_and_emit_options.1.clone();
options.source_map = SourceMapOption::None;
let is_cjs = self.cjs_tracker.is_cjs_with_known_is_script(
specifier,
media_type,
parsed_source.compute_is_script(),
)?;
let transpiled_source = parsed_source
.transpile(
&self.transpile_and_emit_options.0,
&deno_ast::TranspileModuleOptions {
module_kind: Some(module_kind),
module_kind: Some(ModuleKind::from_is_cjs(is_cjs)),
},
&options,
)?
Expand Down
25 changes: 10 additions & 15 deletions cli/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ use crate::npm::CliNpmResolverCreateOptions;
use crate::npm::CliNpmResolverManagedSnapshotOption;
use crate::npm::CreateInNpmPkgCheckerOptions;
use crate::resolver::CjsTracker;
use crate::resolver::CjsTrackerOptions;
use crate::resolver::CliDenoResolverFs;
use crate::resolver::CliGraphResolver;
use crate::resolver::CliGraphResolverOptions;
use crate::resolver::CliNodeResolver;
use crate::resolver::CliResolver;
use crate::resolver::CliResolverOptions;
use crate::resolver::CliSloppyImportsResolver;
use crate::resolver::IsCjsResolverOptions;
use crate::resolver::NpmModuleLoader;
use crate::resolver::SloppyImportsCachedFs;
use crate::standalone::DenoCompileBinaryWriter;
Expand Down Expand Up @@ -201,7 +201,7 @@ struct CliFactoryServices {
parsed_source_cache: Deferred<Arc<ParsedSourceCache>>,
permission_desc_parser: Deferred<Arc<RuntimePermissionDescriptorParser>>,
pkg_json_resolver: Deferred<Arc<PackageJsonResolver>>,
resolver: Deferred<Arc<CliGraphResolver>>,
resolver: Deferred<Arc<CliResolver>>,
root_cert_store_provider: Deferred<Arc<dyn RootCertStoreProvider>>,
root_permissions_container: Deferred<PermissionsContainer>,
sloppy_imports_resolver: Deferred<Option<Arc<CliSloppyImportsResolver>>>,
Expand Down Expand Up @@ -523,14 +523,14 @@ impl CliFactory {
.await
}

pub async fn resolver(&self) -> Result<&Arc<CliGraphResolver>, AnyError> {
pub async fn resolver(&self) -> Result<&Arc<CliResolver>, AnyError> {
self
.services
.resolver
.get_or_try_init_async(
async {
let cli_options = self.cli_options()?;
Ok(Arc::new(CliGraphResolver::new(CliGraphResolverOptions {
Ok(Arc::new(CliResolver::new(CliResolverOptions {
sloppy_imports_resolver: self.sloppy_imports_resolver()?.cloned(),
node_resolver: Some(self.cli_node_resolver().await?.clone()),
npm_resolver: if cli_options.no_npm() {
Expand All @@ -541,9 +541,6 @@ impl CliFactory {
workspace_resolver: self.workspace_resolver().await?.clone(),
bare_node_builtins_enabled: cli_options
.unstable_bare_node_builtins(),
maybe_jsx_import_source_config: cli_options
.workspace()
.to_maybe_jsx_import_source_config()?,
maybe_vendor_dir: cli_options.vendor_dir_path(),
})))
}
Expand Down Expand Up @@ -652,7 +649,6 @@ impl CliFactory {
self.cjs_tracker()?.clone(),
self.fs().clone(),
Some(self.parsed_source_cache().clone()),
self.cli_options()?.is_npm_main(),
);

Ok(Arc::new(NodeCodeTranslator::new(
Expand Down Expand Up @@ -706,6 +702,7 @@ impl CliFactory {
let cli_options = self.cli_options()?;
Ok(Arc::new(ModuleGraphBuilder::new(
self.caches()?.clone(),
self.cjs_tracker()?.clone(),
cli_options.clone(),
self.file_fetcher()?.clone(),
self.fs().clone(),
Expand Down Expand Up @@ -794,8 +791,9 @@ impl CliFactory {
Ok(Arc::new(CjsTracker::new(
self.in_npm_pkg_checker()?.clone(),
self.pkg_json_resolver().clone(),
CjsTrackerOptions {
unstable_detect_cjs: options.unstable_detect_cjs(),
IsCjsResolverOptions {
detect_cjs: options.detect_cjs(),
is_node_main: options.is_node_main(),
},
)))
})
Expand All @@ -809,7 +807,6 @@ impl CliFactory {
.cli_node_resolver
.get_or_try_init_async(async {
Ok(Arc::new(CliNodeResolver::new(
self.cjs_tracker()?.clone(),
self.fs().clone(),
self.in_npm_pkg_checker()?.clone(),
self.node_resolver().await?.clone(),
Expand Down Expand Up @@ -950,10 +947,8 @@ impl CliFactory {
let create_hmr_runner = if cli_options.has_hmr() {
let watcher_communicator = self.watcher_communicator.clone().unwrap();
let emitter = self.emitter()?.clone();
let cjs_tracker = self.cjs_tracker()?.clone();
let fn_: crate::worker::CreateHmrRunnerCb = Box::new(move |session| {
Box::new(HmrRunner::new(
cjs_tracker.clone(),
emitter.clone(),
session,
watcher_communicator.clone(),
Expand Down
Loading