feat(node): stabilize detecting if CJS via `"type": "commonjs"` in a package.json #26439

dsherret · 2024-10-20T20:35:22Z

Stabilizes --unstable-detect-cjs.

This will respect "type": "commonjs" in a package.json to determine if .js/.jsx/.ts/.tsx` files are CJS or ESM. If the file is found to be ESM it will be loaded as ESM though.

Breaking change to remove referrer based CJS/ESM assignment

An extremely important part of module resolution is the referrer should never impact if a file is considered ESM or CJS. This should be done solely based on the file itself without any other context (in other words, it should only be based on its location or its contents).

It seems that require(...) in Deno was treating .js modules outside a node_modules folder as CJS and then falling back to ESM. This means that Deno could potentially load the same module twice. This fix partly fixes #26438 but it seems like there's still something else going on still.

This bug fix will break some existing code, but it will also make some other code work. The code that gets broken by this can be fixed by adding an explicit "type": "commonjs" to the closest package.json file.

Todo:

fix: improved support for cjs and cts modules #26558 (comment)
Cache busting is cjs cache in lsp (refactored to not use a cache in the lsp)

Closes #26225
Closes #26742

nathanwhit · 2024-10-20T23:15:31Z

We should check how this affects popular frameworks/libraries before merging

dsherret · 2024-10-21T03:43:48Z

Yeah I think we’ll have to wait to fix this one with the stabilization of unstable-detect-cjs

arnauorriols

LGTM

Mistake

…er needs to go

dsherret · 2024-11-12T17:05:06Z

cli/args/mod.rs

          }
          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())?


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

dsherret · 2024-11-12T17:10:42Z

cli/lsp/documents.rs

@@ -797,6 +832,7 @@ impl FileSystemDocuments {
  pub fn get(
    &self,
    specifier: &ModuleSpecifier,
+    is_cjs_resolver: &LspIsCjsResolver,


We need to split up data/storage and services that act on that data in the lsp. Passing around these "services" as method parameters (horizontal movement) is bad because it makes introducing new functionality tedious, verbose, and noisy. I've continued doing this here though because it would be too much of a refactor for this PR.

That's reasonable - maybe open an issue to refactor it and add it to Q4 project board?

Opened #26847

dsherret · 2024-11-12T17:13:12Z

cli/lsp/resolver.rs

@@ -182,6 +185,8 @@ impl LspScopeResolver {
              .resolve_req_reference(
                &req_ref,
                &referrer,
+                // todo(dsherret): this is wrong because it doesn't consider CJS referrers
+                NodeModuleKind::Esm,


This is ok for now :)

Ideally we'd store this keyed on NodeModuleKind as well because resolution could differ.

Open an issue for this TODO, especially if you have an example program that will not work correctly in the LSP

I opened #26846

dsherret · 2024-11-12T17:15:14Z

cli/lsp/resolver.rs

-#[derive(Debug)]
-pub struct LspCjsTracker {
-  cjs_tracker: Arc<CjsTracker>,
-}


We don't use the cjs tracker in the lsp anymore. Instead we inspect the state of the documents each time.

dsherret · 2024-11-12T17:17:46Z

cli/tsc/99_main_compiler.js

@@ -732,7 +735,7 @@ delete Object.prototype.__proto__;
      /** @type {Array<[string, ts.Extension] | undefined>} */
      const resolved = ops.op_resolve(
        base,
-        isCjsCache.get(base) ?? false,
+        containingSourceFile?.impliedNodeFormat === ts.ModuleKind.CommonJS,


Doing this is better.

ext/node/ops/require.rs

cli/args/flags.rs

bartlomieju · 2024-11-12T22:16:08Z

cli/args/mod.rs

-    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()


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

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.

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

bartlomieju · 2024-11-12T22:41:46Z

cli/lsp/resolver.rs

@@ -182,6 +185,8 @@ impl LspScopeResolver {
              .resolve_req_reference(
                &req_ref,
                &referrer,
+                // todo(dsherret): this is wrong because it doesn't consider CJS referrers
+                NodeModuleKind::Esm,


Open an issue for this TODO, especially if you have an example program that will not work correctly in the LSP

cli/standalone/mod.rs

…ary main entrypoints, and various small improvements/tests

dsherret · 2024-11-13T00:49:10Z

cli/args/mod.rs

+      Ok(None)
+    }
+  }
+}


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.

dsherret added 6 commits October 20, 2024 16:20

fix(node): requiring a .js file in Deno code should be treated as ESM

bcfad0d

remove todo

d69a3e9

lint

ef56f5e

fix node compat tests probably

3afd732

Maybe fix.

6953eab

fix spec failures

ea9002a

dsherret added this to the 2.1.0 milestone Oct 21, 2024

arnauorriols previously approved these changes Oct 21, 2024

View reviewed changes

Merge branch 'main' into fix_require_js_esm

cf3d987

dsherret mentioned this pull request Oct 30, 2024

fix: improved support for cjs and cts modules #26558

Merged

dsherret marked this pull request as draft November 1, 2024 15:00

dsherret added 4 commits November 8, 2024 10:28

Merge branch 'main' into fix_require_js_esm

57273d9

fixes

ebd12a5

stabilize detect-cjs

72107f3

Some fixes for detect-cjs

848dc74

dsherret changed the title ~~fix(node): requiring a .js file in Deno code should be treated as ESM (except with --unstable-detect-cjs)~~ feat(node): stabilize detecting if CJS via "type": "commonjs" in a package.json Nov 8, 2024

dsherret changed the title ~~feat(node): stabilize detecting if CJS via "type": "commonjs" in a package.json~~ feat(node): stabilize detecting if CJS via "type": "commonjs" in a package.json Nov 8, 2024

remove npm main

9f44576

dsherret added the ci-draft Run the CI on draft PRs. label Nov 8, 2024

dsherret added 8 commits November 8, 2024 14:57

Fix.

d509fda

Starting to separate out cjs tracker, but realizing that graph resolv…

16b3b4a

…er needs to go

Merge branch 'main' into fix_require_js_esm

3f064b9

Figuring this out... CjsTracker should not be used in the lsp

982d8c4

compiling

af5a374

fix tests

e3134c0

fix issue with type checking not resolving based on package.json

c8f5141

switch eval to mts

63f6255

dsherret added 2 commits November 12, 2024 11:45

fix mac

1ff3573

add final lsp test

fa3bd0e

dsherret marked this pull request as ready for review November 12, 2024 18:03

Merge branch 'main' into fix_require_js_esm

c242e5c

dsherret commented Nov 12, 2024

View reviewed changes

fix

c84900e

dsherret removed the ci-draft Run the CI on draft PRs. label Nov 12, 2024

dsherret requested review from arnauorriols, bartlomieju and nathanwhit November 12, 2024 19:09

dsherret commented Nov 12, 2024

View reviewed changes

ext/node/ops/require.rs Outdated Show resolved Hide resolved

bartlomieju reviewed Nov 12, 2024

View reviewed changes

dsherret added 2 commits November 12, 2024 18:51

Align require node module kind detection, handle resolving node bin…

fe03ea7

…ary main entrypoints, and various small improvements/tests

Merge branch 'main' into fix_require_js_esm

d1184c7

This was referenced Nov 13, 2024

LSP - Better mapping between specifier and a package.json dep #26846

Open

Refactor lsp to separate service structs from data/state #26847

Open

dsherret requested a review from bartlomieju November 13, 2024 00:47

dsherret commented Nov 13, 2024

View reviewed changes

bartlomieju approved these changes Nov 13, 2024

View reviewed changes

dsherret added 2 commits November 13, 2024 09:31

add comment

c4256e9

Merge branch 'main' into fix_require_js_esm

0014ec6

dsherret enabled auto-merge (squash) November 13, 2024 14:32

dsherret merged commit f091d1a into denoland:main Nov 13, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node): stabilize detecting if CJS via `"type": "commonjs"` in a package.json #26439

feat(node): stabilize detecting if CJS via `"type": "commonjs"` in a package.json #26439

dsherret commented Oct 20, 2024 •

edited

Loading

nathanwhit commented Oct 20, 2024

dsherret commented Oct 21, 2024

arnauorriols left a comment

dsherret Nov 12, 2024

dsherret Nov 12, 2024

bartlomieju Nov 12, 2024

dsherret Nov 13, 2024

dsherret Nov 12, 2024

bartlomieju Nov 12, 2024

dsherret Nov 13, 2024

dsherret Nov 12, 2024

dsherret Nov 12, 2024

bartlomieju Nov 12, 2024

dsherret Nov 12, 2024

bartlomieju Nov 13, 2024

bartlomieju Nov 12, 2024

dsherret Nov 13, 2024

feat(node): stabilize detecting if CJS via "type": "commonjs" in a package.json #26439

feat(node): stabilize detecting if CJS via "type": "commonjs" in a package.json #26439

Conversation

dsherret commented Oct 20, 2024 • edited Loading

nathanwhit commented Oct 20, 2024

dsherret commented Oct 21, 2024

arnauorriols left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(node): stabilize detecting if CJS via `"type": "commonjs"` in a package.json #26439

feat(node): stabilize detecting if CJS via `"type": "commonjs"` in a package.json #26439

dsherret commented Oct 20, 2024 •

edited

Loading