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 21 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
21 changes: 5 additions & 16 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,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 @@ -1142,14 +1141,14 @@ impl CliOptions {
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())?
}
Expand Down Expand Up @@ -1207,14 +1206,6 @@ impl CliOptions {
}
}

// If the main module should be treated as being in an npm package.
// 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 {
NPM_PROCESS_STATE.is_some()
}

pub fn has_node_modules_dir(&self) -> bool {
self.maybe_node_modules_folder.is_some()
}
Expand Down Expand Up @@ -1589,9 +1580,8 @@ 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 {
self.workspace().package_jsons().next().is_some()
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct? Doesn't it check for existence of package.json and not consulting if it has type: "commonjs"?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only to activate the feature of it searching for the closest package.json files on every ambiguous module load. That means only projects with a package.json get opted into this feature and other Deno programs won't have the perf penalty of the extra work.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, consider adding a docstring here to avoid confusion in the future.

}

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

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
24 changes: 9 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,8 @@ 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(),
},
)))
})
Expand All @@ -809,7 +806,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 @@ -949,10 +945,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
79 changes: 72 additions & 7 deletions cli/graph_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,19 @@ use crate::colors;
use crate::errors::get_error_class_name;
use crate::file_fetcher::FileFetcher;
use crate::npm::CliNpmResolver;
use crate::resolver::CliGraphResolver;
use crate::resolver::CjsTracker;
use crate::resolver::CliResolver;
use crate::resolver::CliSloppyImportsResolver;
use crate::resolver::SloppyImportsCachedFs;
use crate::tools::check;
use crate::tools::check::TypeChecker;
use crate::util::file_watcher::WatcherCommunicator;
use crate::util::fs::canonicalize_path;
use deno_config::deno_json::JsxImportSourceConfig;
use deno_config::workspace::JsrPackageConfig;
use deno_core::anyhow::bail;
use deno_graph::source::LoaderChecksum;
use deno_graph::source::ResolutionMode;
use deno_graph::FillFromLockfileOptions;
use deno_graph::JsrLoadError;
use deno_graph::ModuleLoadError;
Expand Down Expand Up @@ -379,6 +382,7 @@ pub struct BuildFastCheckGraphOptions<'a> {

pub struct ModuleGraphBuilder {
caches: Arc<cache::Caches>,
cjs_tracker: Arc<CjsTracker>,
cli_options: Arc<CliOptions>,
file_fetcher: Arc<FileFetcher>,
fs: Arc<dyn FileSystem>,
Expand All @@ -389,14 +393,15 @@ pub struct ModuleGraphBuilder {
module_info_cache: Arc<ModuleInfoCache>,
npm_resolver: Arc<dyn CliNpmResolver>,
parsed_source_cache: Arc<ParsedSourceCache>,
resolver: Arc<CliGraphResolver>,
resolver: Arc<CliResolver>,
root_permissions_container: PermissionsContainer,
}

impl ModuleGraphBuilder {
#[allow(clippy::too_many_arguments)]
pub fn new(
caches: Arc<cache::Caches>,
cjs_tracker: Arc<CjsTracker>,
cli_options: Arc<CliOptions>,
file_fetcher: Arc<FileFetcher>,
fs: Arc<dyn FileSystem>,
Expand All @@ -407,11 +412,12 @@ impl ModuleGraphBuilder {
module_info_cache: Arc<ModuleInfoCache>,
npm_resolver: Arc<dyn CliNpmResolver>,
parsed_source_cache: Arc<ParsedSourceCache>,
resolver: Arc<CliGraphResolver>,
resolver: Arc<CliResolver>,
root_permissions_container: PermissionsContainer,
) -> Self {
Self {
caches,
cjs_tracker,
cli_options,
file_fetcher,
fs,
Expand Down Expand Up @@ -518,7 +524,7 @@ impl ModuleGraphBuilder {
None => MutLoaderRef::Owned(self.create_graph_loader()),
};
let cli_resolver = &self.resolver;
let graph_resolver = cli_resolver.as_graph_resolver();
let graph_resolver = self.create_graph_resolver()?;
let graph_npm_resolver = cli_resolver.create_graph_npm_resolver();
let maybe_file_watcher_reporter = self
.maybe_file_watcher_reporter
Expand All @@ -543,7 +549,7 @@ impl ModuleGraphBuilder {
npm_resolver: Some(&graph_npm_resolver),
module_analyzer: &analyzer,
reporter: maybe_file_watcher_reporter,
resolver: Some(graph_resolver),
resolver: Some(&graph_resolver),
locker: locker.as_mut().map(|l| l as _),
},
)
Expand Down Expand Up @@ -666,7 +672,7 @@ impl ModuleGraphBuilder {
};
let parser = self.parsed_source_cache.as_capturing_parser();
let cli_resolver = &self.resolver;
let graph_resolver = cli_resolver.as_graph_resolver();
let graph_resolver = self.create_graph_resolver()?;
let graph_npm_resolver = cli_resolver.create_graph_npm_resolver();

graph.build_fast_check_type_graph(
Expand All @@ -675,7 +681,7 @@ impl ModuleGraphBuilder {
fast_check_cache: fast_check_cache.as_ref().map(|c| c as _),
fast_check_dts: false,
jsr_url_provider: &CliJsrUrlProvider,
resolver: Some(graph_resolver),
resolver: Some(&graph_resolver),
npm_resolver: Some(&graph_npm_resolver),
workspace_fast_check: options.workspace_fast_check,
},
Expand Down Expand Up @@ -739,6 +745,18 @@ impl ModuleGraphBuilder {
},
)
}

fn create_graph_resolver(&self) -> Result<CliGraphResolver, AnyError> {
let jsx_import_source_config = self
.cli_options
.workspace()
.to_maybe_jsx_import_source_config()?;
Ok(CliGraphResolver {
cjs_tracker: &self.cjs_tracker,
resolver: &self.resolver,
jsx_import_source_config,
})
}
}

/// Adds more explanatory information to a resolution error.
Expand Down Expand Up @@ -1143,6 +1161,53 @@ fn format_deno_graph_error(err: &dyn Error) -> String {
message
}

#[derive(Debug)]
struct CliGraphResolver<'a> {
cjs_tracker: &'a CjsTracker,
resolver: &'a CliResolver,
jsx_import_source_config: Option<JsxImportSourceConfig>,
}

impl<'a> deno_graph::source::Resolver for CliGraphResolver<'a> {
fn default_jsx_import_source(&self) -> Option<String> {
self
.jsx_import_source_config
.as_ref()
.and_then(|c| c.default_specifier.clone())
}

fn default_jsx_import_source_types(&self) -> Option<String> {
self
.jsx_import_source_config
.as_ref()
.and_then(|c| c.default_types_specifier.clone())
}

fn jsx_import_source_module(&self) -> &str {
self
.jsx_import_source_config
.as_ref()
.map(|c| c.module.as_str())
.unwrap_or(deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE)
}

fn resolve(
&self,
raw_specifier: &str,
referrer_range: &deno_graph::Range,
mode: ResolutionMode,
) -> Result<ModuleSpecifier, ResolveError> {
self.resolver.resolve(
raw_specifier,
referrer_range,
self
.cjs_tracker
.get_referrer_kind(&referrer_range.specifier),
mode,
)
}
}

#[cfg(test)]
mod test {
use std::sync::Arc;
Expand Down
Loading
Loading