From a923f4e864b2975b46f7c20bcc7c702ac0d5287d Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Mon, 4 Nov 2024 17:11:50 -0800 Subject: [PATCH] fix(compartment-mapper): top-level "this" is "module.exports" (#2620) For CJS compat, the top-level `this` must be equal to `module.exports` (which must be equal to `exports`). This implementation changes the CJS parsers to call the wrapper `functor` using the context of `module.exports`. Fixed an unrelated "dynamic require" test so it does not use the CJS compat fixtures. --- packages/compartment-mapper/src/parse-cjs.js | 10 +++++++++- .../compartment-mapper/src/parse-pre-cjs.js | 10 +++++++++- .../test/dynamic-require.test.js | 4 ++-- .../node_modules/app/index.js | 4 ++++ .../exports-shenanigans/exports-this.js | 4 ++++ .../node_modules/static-app/README.md | 1 + .../node_modules/static-app/index.js | 1 + .../node_modules/static-app/package.json | 12 ++++++++++++ .../test/snapshots/error-handling.test.js.md | 8 ++++---- .../test/snapshots/error-handling.test.js.snap | Bin 799 -> 805 bytes 10 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 packages/compartment-mapper/test/fixtures-cjs-compat/node_modules/exports-shenanigans/exports-this.js create mode 100644 packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/README.md create mode 100644 packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/index.js create mode 100644 packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/package.json diff --git a/packages/compartment-mapper/src/parse-cjs.js b/packages/compartment-mapper/src/parse-cjs.js index b440479634..636be07ec6 100644 --- a/packages/compartment-mapper/src/parse-cjs.js +++ b/packages/compartment-mapper/src/parse-cjs.js @@ -51,7 +51,15 @@ export const parseCjs = ( readPowers, }); - functor(require, moduleExports, module, filename, dirname); + // In CommonJS, the top-level `this` is the `module.exports` object. + functor.call( + moduleExports, + require, + moduleExports, + module, + filename, + dirname, + ); afterExecute(); }; diff --git a/packages/compartment-mapper/src/parse-pre-cjs.js b/packages/compartment-mapper/src/parse-pre-cjs.js index b94d47df4b..c868f3e963 100644 --- a/packages/compartment-mapper/src/parse-pre-cjs.js +++ b/packages/compartment-mapper/src/parse-pre-cjs.js @@ -43,7 +43,15 @@ export const parsePreCjs = ( readPowers, }); - functor(require, moduleExports, module, filename, dirname); + // In CommonJS, the top-level `this` is the `module.exports` object. + functor.call( + moduleExports, + require, + moduleExports, + module, + filename, + dirname, + ); afterExecute(); }; diff --git a/packages/compartment-mapper/test/dynamic-require.test.js b/packages/compartment-mapper/test/dynamic-require.test.js index e2e2694e05..cb3ff0618b 100644 --- a/packages/compartment-mapper/test/dynamic-require.test.js +++ b/packages/compartment-mapper/test/dynamic-require.test.js @@ -391,7 +391,7 @@ test('sync module transforms work with dynamic require support', async t => { test('sync module transforms work without dynamic require support', async t => { const fixture = new URL( - 'fixtures-cjs-compat/node_modules/app/index.js', + 'fixtures-dynamic/node_modules/static-app/index.js', import.meta.url, ).toString(); @@ -413,5 +413,5 @@ test('sync module transforms work without dynamic require support', async t => { syncModuleTransforms, }); - t.true(transformCount === 29); + t.is(transformCount, 2); }); diff --git a/packages/compartment-mapper/test/fixtures-cjs-compat/node_modules/app/index.js b/packages/compartment-mapper/test/fixtures-cjs-compat/node_modules/app/index.js index 864ef2f832..9947a3880e 100644 --- a/packages/compartment-mapper/test/fixtures-cjs-compat/node_modules/app/index.js +++ b/packages/compartment-mapper/test/fixtures-cjs-compat/node_modules/app/index.js @@ -1,5 +1,6 @@ const exportsShenanigans = require('exports-shenanigans'); const fromDebug = require('exports-shenanigans/from-debug'); +const exportsThis = require('exports-shenanigans/exports-this'); const interops = require('interops'); const moduleinterops = require('./moduleinterops'); const stableOrderJson = require('./assertion-generators/json.cjs'); @@ -92,5 +93,8 @@ module.exports.assertions = { if(!fromDebug.isOk) { throw Error(`Overwriting exports with derivation of exports should work. exports-shenanigans/from-debug fixture did not load.`) } + if (exportsThis.magicNumber !== 2) { + throw Error('Top-level "this" should be a reference to module.exports'); + } }, }; diff --git a/packages/compartment-mapper/test/fixtures-cjs-compat/node_modules/exports-shenanigans/exports-this.js b/packages/compartment-mapper/test/fixtures-cjs-compat/node_modules/exports-shenanigans/exports-this.js new file mode 100644 index 0000000000..796b2d9aef --- /dev/null +++ b/packages/compartment-mapper/test/fixtures-cjs-compat/node_modules/exports-shenanigans/exports-this.js @@ -0,0 +1,4 @@ +exports.magicNumber = 1; + +// exports === this +this.magicNumber++; diff --git a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/README.md b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/README.md new file mode 100644 index 0000000000..05e59c4ba2 --- /dev/null +++ b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/README.md @@ -0,0 +1 @@ +This is the "control" which does not use dynamic requires. \ No newline at end of file diff --git a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/index.js b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/index.js new file mode 100644 index 0000000000..bbad3566d4 --- /dev/null +++ b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/index.js @@ -0,0 +1 @@ +exports.isOk = require('is-ok').isOk; \ No newline at end of file diff --git a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/package.json b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/package.json new file mode 100644 index 0000000000..9d65d69fa7 --- /dev/null +++ b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/static-app/package.json @@ -0,0 +1,12 @@ +{ + "name": "app", + "version": "1.0.0", + "main": "./index.js", + "type": "commonjs", + "dependencies": { + "is-ok": "^1.0.0" + }, + "scripts": { + "preinstall": "echo DO NOT INSTALL TEST FIXTURES; exit -1" + } +} diff --git a/packages/compartment-mapper/test/snapshots/error-handling.test.js.md b/packages/compartment-mapper/test/snapshots/error-handling.test.js.md index 9b368bdb31..be0ed22671 100644 --- a/packages/compartment-mapper/test/snapshots/error-handling.test.js.md +++ b/packages/compartment-mapper/test/snapshots/error-handling.test.js.md @@ -112,7 +112,7 @@ Generated by [AVA](https://avajs.dev). at compartmentImportNow (file://.../ses/src/compartment.js:…)␊ at Compartment.importNow (file://.../ses/src/compartment.js:…)␊ at require (file://.../compartment-mapper/src/parse-cjs-shared-export-wrapper.js:…)␊ - at eval (eval at (eval at makeEvaluate (file://.../ses/src/make-evaluate.js:…)), :…)␊ + at Proxy.eval (eval at (eval at makeEvaluate (file://.../ses/src/make-evaluate.js:…)), :…)␊ at Object.execute (file://.../compartment-mapper/src/parse-pre-cjs.js:…)␊ at execute (file://.../ses/src/module-instance.js:…)␊ at compartmentImportNow (file://.../ses/src/compartment.js:…)␊ @@ -128,7 +128,7 @@ Generated by [AVA](https://avajs.dev). at compartmentImportNow (file://.../ses/src/compartment.js:…)␊ at Compartment.importNow (file://.../ses/src/compartment.js:…)␊ at require (file://.../compartment-mapper/src/parse-cjs-shared-export-wrapper.js:…)␊ - at eval (eval at (eval at makeEvaluate (file://.../ses/src/make-evaluate.js:…)), :…)␊ + at Proxy.eval (eval at (eval at makeEvaluate (file://.../ses/src/make-evaluate.js:…)), :…)␊ at Object.execute (file://.../compartment-mapper/src/parse-pre-cjs.js:…)␊ at execute (file://.../ses/src/module-instance.js:…)␊ at compartmentImportNow (file://.../ses/src/compartment.js:…)␊ @@ -144,7 +144,7 @@ Generated by [AVA](https://avajs.dev). at compartmentImportNow (file://.../ses/src/compartment.js:…)␊ at Compartment.importNow (file://.../ses/src/compartment.js:…)␊ at require (file://.../compartment-mapper/src/parse-cjs-shared-export-wrapper.js:…)␊ - at eval (eval at (eval at makeEvaluate (file://.../ses/src/make-evaluate.js:…)), :…)␊ + at Proxy.eval (eval at (eval at makeEvaluate (file://.../ses/src/make-evaluate.js:…)), :…)␊ at Object.execute (file://.../compartment-mapper/src/parse-pre-cjs.js:…)␊ at execute (file://.../ses/src/module-instance.js:…)␊ at compartmentImportNow (file://.../ses/src/compartment.js:…)␊ @@ -160,7 +160,7 @@ Generated by [AVA](https://avajs.dev). at compartmentImportNow (file://.../ses/src/compartment.js:…)␊ at Compartment.importNow (file://.../ses/src/compartment.js:…)␊ at require (file://.../compartment-mapper/src/parse-cjs-shared-export-wrapper.js:…)␊ - at eval (eval at (eval at makeEvaluate (file://.../ses/src/make-evaluate.js:…)), :…)␊ + at Proxy.eval (eval at (eval at makeEvaluate (file://.../ses/src/make-evaluate.js:…)), :…)␊ at Object.execute (file://.../compartment-mapper/src/parse-pre-cjs.js:…)␊ at execute (file://.../ses/src/module-instance.js:…)␊ at compartmentImportNow (file://.../ses/src/compartment.js:…)␊ diff --git a/packages/compartment-mapper/test/snapshots/error-handling.test.js.snap b/packages/compartment-mapper/test/snapshots/error-handling.test.js.snap index 429e695ad84223dfaf3011f8e58541da9af78462..d5b62f73817fe30af18fff6e065f7baa7a8f0d14 100644 GIT binary patch literal 805 zcmV+=1KRvSRzVGzmQ8$Wg?BF zwO709?#hQvr@rU8uIu?S zNl}@EgmDr~Q&OHGZMIoUp$4Xv%q!X_P+s`2r0VzHy_7WoFMPMHePzY%tWFi2{ z4Jm>AVr2=_nu(~fuG?JJLjV9YU}7c5`%$DQLQ`c6wrdk<(wBJdzZ zrKgWqjo00681Grv8gIjm7qE!5IZ`rtjcG$-#{LRLtZMIYW@xr#X&4b4qmIvwa{Kas zL3e!!9sAU=M{9lSF3v#h)JwlO2wbXLwyM zaYzoGkC;xgvs+hd%)ETDtF5J~+}?(jd7;q@wE5bIZf)~fzAfLD@AdNCdSWltHMB%q jqAk(uC3;S;a3;}hy+X^hW!f^m9;SZ-hE-XI(I)@^sS}Bp literal 799 zcmV+)1K|8YRzVy#!?_xQu}TnG_fZh2c9Dpyf((BQZ0HvgX@)7^20`txkLE$yRYa zdc5_#^`dn$&gigE>cSCnK_OUGYCx*OTo818mLiva&vbxG$S~+*OwftApe|#+@3TOq z8ETu7v|fr?Mw(eNmff&aCRIpZDtW`mq^twmuOXKqX)!ML5+N5wU>6QhN@XG9G6Wh~ z(h|kx%2I9&mvL=fce$*m003ygCRz>m<5*LSw#pW4PfcXVFrx!j#d<~d{GRFlKKr#Z zktj2|WDJ_I3>Mjt2{4M3mVk7wRdaAd)0Qv+i>HBJSJn0v`XJ&R+E^qzd{kK+Ixaqnk`uxMux{|k_)5Uz5GAW zT_1wSt0M?QZWBPrG(~*mfOJ4QAn)N!9BOW-h%Bmk^O^X)hTy;<=n#A;2u}O_KVf@s zNw%wREl0Pb+tI!GbkFJ11wGT;rF&VU`C03NQvKOQ zD(@#6(@Y62X+@Yca``Eze8u>O*fE*Yo~u08*q(DPb1u8ZH%R5OrTTJt&2dqClN|T) zv`2yUX+%M8X$jZOJBVhfU8y>k#-a?ag-%WSww&mL8qU<28hBOXVCpZvs)(kqIoAti zq%6M|^o(Z`)D(I&DpNGn<(O;P8K8je@{ShvP%1f2Rc_w={jspu;Q&RBH)Rp~>7_b_j%CNPV|iUH d&*==#WVxv`=xBB{JDTsC=0A0$0p7VM002RAgG&Ga