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(unstable): repurpose --unstable-detect-cjs to attempt loading more modules as cjs #27094

Merged
merged 18 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion cli/args/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,7 @@ 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,
pub sloppy_imports: bool,
pub features: Vec<String>, // --unstabe-kv --unstable-cron
}
Expand Down Expand Up @@ -4373,7 +4374,7 @@ impl CommandExt for Command {
).arg(
Arg::new("unstable-detect-cjs")
.long("unstable-detect-cjs")
.help("Reads the package.json type field in a project to treat .js files as .cjs")
.help("Treats ambiguous .js, .jsx, .ts, .tsx files as CommonJS modules in more cases")
.value_parser(FalseyValueParser::new())
.action(ArgAction::SetTrue)
.hide(true)
Copy link
Member

Choose a reason for hiding this comment

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

Unhide?

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 thought so too, but all of the unstable flags seem hidden

Copy link
Member

Choose a reason for hiding this comment

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

Oh, just make sure it's shown when you do deno run --help=unstable

Expand Down Expand Up @@ -5986,6 +5987,7 @@ 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
6 changes: 6 additions & 0 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1606,6 +1606,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
Expand Down Expand Up @@ -1675,6 +1680,7 @@ impl CliOptions {
"sloppy-imports",
"byonm",
"bare-node-builtins",
"detect-cjs",
"fmt-component",
"fmt-sql",
])
Expand Down
11 changes: 7 additions & 4 deletions cli/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ use crate::resolver::CliNpmReqResolver;
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 All @@ -72,6 +71,7 @@ use deno_core::error::AnyError;
use deno_core::futures::FutureExt;
use deno_core::FeatureChecker;

use deno_resolver::cjs::IsCjsResolutionMode;
use deno_resolver::npm::NpmReqResolverOptions;
use deno_resolver::DenoResolverOptions;
use deno_resolver::NodeAndNpmReqResolver;
Expand Down Expand Up @@ -845,9 +845,12 @@ impl CliFactory {
Ok(Arc::new(CjsTracker::new(
self.in_npm_pkg_checker()?.clone(),
self.pkg_json_resolver().clone(),
IsCjsResolverOptions {
detect_cjs: options.detect_cjs(),
is_node_main: options.is_node_main(),
if options.is_node_main() || options.unstable_detect_cjs() {
IsCjsResolutionMode::ImplicitTypeCommonJs
} else if options.detect_cjs() {
IsCjsResolutionMode::ExplicitTypeCommonJs
} else {
IsCjsResolutionMode::Disabled
},
)))
})
Expand Down
8 changes: 3 additions & 5 deletions cli/lsp/analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1281,9 +1281,7 @@ impl CodeActionCollection {
import_start_from_specifier(document, i)
})?;
let referrer = document.specifier();
let referrer_kind = language_server
.is_cjs_resolver
.get_doc_resolution_mode(document);
let resolution_mode = document.resolution_mode();
let file_referrer = document.file_referrer();
let config_data = language_server
.config
Expand All @@ -1309,7 +1307,7 @@ impl CodeActionCollection {
if !language_server.resolver.is_bare_package_json_dep(
&dep_key,
referrer,
referrer_kind,
resolution_mode,
) {
return None;
}
Expand All @@ -1329,7 +1327,7 @@ impl CodeActionCollection {
}
if language_server
.resolver
.npm_to_file_url(&npm_ref, referrer, referrer_kind, file_referrer)
.npm_to_file_url(&npm_ref, referrer, resolution_mode, file_referrer)
.is_some()
{
// The package import has types.
Expand Down
4 changes: 1 addition & 3 deletions cli/lsp/completions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use super::jsr::CliJsrSearchApi;
use super::lsp_custom;
use super::npm::CliNpmSearchApi;
use super::registries::ModuleRegistry;
use super::resolver::LspIsCjsResolver;
use super::resolver::LspResolver;
use super::search::PackageSearchApi;
use super::tsc;
Expand Down Expand Up @@ -161,7 +160,6 @@ pub async fn get_import_completions(
jsr_search_api: &CliJsrSearchApi,
npm_search_api: &CliNpmSearchApi,
documents: &Documents,
is_cjs_resolver: &LspIsCjsResolver,
resolver: &LspResolver,
maybe_import_map: Option<&ImportMap>,
) -> Option<lsp::CompletionResponse> {
Expand All @@ -171,7 +169,7 @@ pub async fn get_import_completions(
let resolution_mode = graph_range
.resolution_mode
.map(to_node_resolution_mode)
.unwrap_or_else(|| is_cjs_resolver.get_doc_resolution_mode(&document));
.unwrap_or_else(|| document.resolution_mode());
let range = to_narrow_lsp_range(document.text_info(), graph_range.range);
let resolved = resolver
.as_cli_resolver(file_referrer)
Expand Down
1 change: 0 additions & 1 deletion cli/lsp/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1707,7 +1707,6 @@ mod tests {
documents: Arc::new(documents),
assets: Default::default(),
config: Arc::new(config),
is_cjs_resolver: Default::default(),
resolver,
},
)
Expand Down
Loading