fix: remove node.module recommendation

sanity-io · Mar 7, 2024 · b8bbca3 · b8bbca3
1 parent f79c63d
commit b8bbca3
Show file tree

Hide file tree

Showing 12 changed files with 64 additions and 106 deletions.
diff --git a/playground/default-export/package.json b/playground/default-export/package.json
@@ -4,15 +4,12 @@
   "private": true,
   "license": "MIT",
   "type": "module",
+  "sideEffects": false,
   "exports": {
     ".": {
       "source": "./src/index.js",
-      "require": "./dist/index.cjs",
-      "node": {
-        "module": "./dist/index.js",
-        "import": "./dist/index.cjs.js"
-      },
       "import": "./dist/index.js",
+      "require": "./dist/index.cjs",
       "default": "./dist/index.js"
     },
     "./package.json": "./package.json"

diff --git a/playground/dummy-commonjs/package.json b/playground/dummy-commonjs/package.json
@@ -14,7 +14,6 @@
         "require": "./dist/index.browser.js"
       },
       "node": {
-        "module": "./dist/index.mjs",
         "import": "./node/index.mjs",
         "require": "./node/index.js"
       },
@@ -31,7 +30,6 @@
         "require": "./dist/extra.browser.js"
       },
       "node": {
-        "module": "./dist/extra.mjs",
         "import": "./node/extra.mjs",
         "require": "./node/extra.js"
       },

diff --git a/playground/dummy-module/package.json b/playground/dummy-module/package.json
@@ -14,7 +14,6 @@
         "require": "./dist/index.browser.cjs"
       },
       "node": {
-        "module": "./dist/index.js",
         "import": "./node/index.js",
         "require": "./node/index.cjs"
       },
@@ -31,7 +30,6 @@
         "require": "./dist/extra.browser.cjs"
       },
       "node": {
-        "module": "./dist/extra.js",
         "import": "./node/extra.js",
         "require": "./node/extra.cjs"
       },

diff --git a/playground/multi-export/package.json b/playground/multi-export/package.json
@@ -11,23 +11,15 @@
     ".": {
       "types": "./dist/index.d.ts",
       "source": "./src/index.ts",
-      "require": "./dist/index.cjs",
-      "node": {
-        "module": "./dist/index.js",
-        "import": "./dist/index.cjs.js"
-      },
       "import": "./dist/index.js",
+      "require": "./dist/index.cjs",
       "default": "./dist/index.js"
     },
     "./plugin": {
       "types": "./dist/plugin.d.ts",
       "source": "./src/plugin.ts",
-      "require": "./dist/plugin.cjs",
-      "node": {
-        "module": "./dist/plugin.js",
-        "import": "./dist/plugin.cjs.js"
-      },
       "import": "./dist/plugin.js",
+      "require": "./dist/plugin.cjs",
       "default": "./dist/plugin.js"
     },
     "./pure": {

diff --git a/playground/multi-exports-commonjs/package.json b/playground/multi-exports-commonjs/package.json
@@ -8,23 +8,15 @@
     ".": {
       "types": "./dist/index.d.ts",
       "source": "./src/index.ts",
-      "require": "./dist/index.js",
-      "node": {
-        "module": "./dist/index.esm.js",
-        "import": "./dist/index.cjs.mjs"
-      },
       "import": "./dist/index.esm.js",
+      "require": "./dist/index.js",
       "default": "./dist/index.esm.js"
     },
     "./plugin": {
       "types": "./dist/plugin.d.ts",
       "source": "./src/plugin.ts",
-      "require": "./dist/plugin.js",
-      "node": {
-        "module": "./dist/plugin.esm.js",
-        "import": "./dist/plugin.cjs.mjs"
-      },
       "import": "./dist/plugin.esm.js",
+      "require": "./dist/plugin.js",
       "default": "./dist/plugin.esm.js"
     },
     "./package.json": "./package.json"

diff --git a/playground/ts-bundler/package.json b/playground/ts-bundler/package.json
@@ -8,12 +8,8 @@
     ".": {
       "types": "./dist/index.d.ts",
       "source": "./src/index.ts",
-      "require": "./dist/index.cjs",
-      "node": {
-        "module": "./dist/index.js",
-        "import": "./dist/index.cjs.js"
-      },
       "import": "./dist/index.js",
+      "require": "./dist/index.cjs",
       "default": "./dist/index.js"
     },
     "./package.json": "./package.json"

diff --git a/playground/ts-node16/package.json b/playground/ts-node16/package.json
@@ -8,12 +8,8 @@
     ".": {
       "types": "./dist/index.d.ts",
       "source": "./src/index.ts",
-      "require": "./dist/index.cjs",
-      "node": {
-        "module": "./dist/index.js",
-        "import": "./dist/index.cjs.js"
-      },
       "import": "./dist/index.js",
+      "require": "./dist/index.cjs",
       "default": "./dist/index.js"
     },
     "./package.json": "./package.json"

diff --git a/playground/ts/package.json b/playground/ts/package.json
@@ -8,12 +8,8 @@
     ".": {
       "types": "./dist/index.d.ts",
       "source": "./src/index.ts",
-      "require": "./dist/index.cjs",
-      "node": {
-        "module": "./dist/index.js",
-        "import": "./dist/index.cjs.js"
-      },
       "import": "./dist/index.js",
+      "require": "./dist/index.cjs",
       "default": "./dist/index.js"
     },
     "./package.json": "./package.json"

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
diff --git a/src/node/core/pkg/loadPkgWithReporting.ts b/src/node/core/pkg/loadPkgWithReporting.ts
@@ -34,51 +34,17 @@ export async function loadPkgWithReporting(options: {
         }
 
         if (exp.node) {
-          const nodeKeys = Object.keys(exp.node)
-
-          if (!assertOrder('module', 'import', nodeKeys)) {
-            shouldError = true
-            logger.error(
-              `exports["${expPath}"]: the \`node.module\` property should come before the \`node.import\` property`,
-            )
-          }
-
-          if (!assertOrder('import', 'require', nodeKeys)) {
-            logger.warn(
-              `exports["${expPath}"]: the \`node.import\` property should come before the \`node.require\` property`,
-            )
-          }
-
-          if (!assertOrder('module', 'require', nodeKeys)) {
-            logger.warn(
-              `exports["${expPath}"]: the \`node.module\` property should come before \`node.require\` property`,
-            )
-          }
-
           if (exp.import && exp.node.import && !assertOrder('node', 'import', keys)) {
             shouldError = true
             logger.error(
               `exports["${expPath}"]: the \`node\` property should come before the \`import\` property`,
             )
           }
 
-          if (exp.module && exp.node.module && !assertOrder('node', 'module', keys)) {
-            shouldError = true
-            logger.error(
-              `exports["${expPath}"]: the \`node\` property should come before the \`module\` property`,
-            )
-          }
-
-          if (
-            exp.node.import &&
-            !exp.node.require &&
-            exp.node.module &&
-            exp.import &&
-            exp.node.module !== exp.import
-          ) {
+          if (exp.node.module) {
             shouldError = true
             logger.error(
-              `exports["${expPath}"]: the \`node.module\` property should match \`import\``,
+              `exports["${expPath}"]: the \`node.module\` condition shouldn't be used as it's not well supported in all bundlers. A better strategy is to refactor the codebase to no longer be vulnerable to the "dual package hazard"`,
             )
           }
 

diff --git a/src/node/core/pkg/validatePkg.ts b/src/node/core/pkg/validatePkg.ts
@@ -33,12 +33,10 @@ const pkgSchema = z.object({
           node: z.optional(
             z.object({
               source: z.optional(z.string()),
-              module: z.optional(z.string()),
               import: z.optional(z.string()),
               require: z.optional(z.string()),
             }),
           ),
-          module: z.optional(z.string()),
           import: z.optional(z.string()),
           require: z.optional(z.string()),
           default: z.string(),

diff --git a/test/parseExports.test.ts b/test/parseExports.test.ts
@@ -192,17 +192,6 @@ describe('parseExports', () => {
         },
         `"node" can output both a "import" and "require" condition`,
       ],
-      [
-        {
-          require: './dist/index.cjs',
-          node: {
-            module: './dist/index.js',
-            import: './dist/index.cjs.js',
-          },
-          import: './dist/index.js',
-        },
-        `"node" can be used to implement a wrapper that protects against the "dual package hazard"`,
-      ],
     ])('%o', (json, msg) => {
       const extMap = getPkgExtMap({legacyExports: false})
 
@@ -272,50 +261,58 @@ describe('parseExports', () => {
           import: './dist/index.js',
           node: {
             module: './dist/index.js',
-            import: './dist/index.cjs.js',
           },
           default: './dist/index.js',
         },
-        `"node.import" must be before "import"`,
+        `"node.module" considered harmful`,
       ],
       [
         {
           source: './src/index.ts',
+          module: './dist/index.js',
+          require: './dist/index.cjs',
+          import: './dist/index.js',
+          default: './dist/index.js',
+        },
+        `"module" considered harmful`,
+      ],
+      [
+        {
+          source: './src/index.ts',
+          require: './dist/index.cjs',
+          import: './dist/index.js',
           node: {
             module: './dist/index.js',
             import: './dist/index.cjs.js',
-            require: './dist/index.cjs',
           },
-          import: './dist/index.js',
-          require: './dist/index.cjs',
           default: './dist/index.js',
         },
-        `"node.require" is unnecesary if it's the same as "require"`,
+        `"node.import" must be before "import"`,
       ],
       [
         {
           source: './src/index.ts',
-          require: './dist/index.cjs',
           node: {
+            module: './dist/index.js',
             import: './dist/index.cjs.js',
+            require: './dist/index.cjs',
           },
           import: './dist/index.js',
+          require: './dist/index.cjs',
           default: './dist/index.js',
         },
-        `"node.module" should be specified when a "node.import" dual package hazard is used`,
+        `"node.require" is unnecesary if it's the same as "require"`,
       ],
       [
         {
-          source: './src/index.ts',
           require: './dist/index.cjs',
-          module: './dist/index.js',
           node: {
+            module: './dist/index.js',
             import: './dist/index.cjs.js',
           },
           import: './dist/index.js',
-          default: './dist/index.js',
         },
-        `"module" should be moved to "node.module" when it's the same as "import"`,
+        `the "node" re-export pattern considered harmful, protect against the "dual package hazard" in ways that have high ecosystem compatibility`,
       ],
       [
         {