sadpandajoe commented on code in PR #45595: URL: https://github.com/apache/superset/pull/45595#discussion_r4373966058
########## superset-frontend/spec/helpers/swcJestTransformer.js: ########## @@ -0,0 +1,147 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { createTransformer as createSwcTransformer } from '@swc/jest'; + +/** + * Jest transformer: @swc/jest plus one post-processing step. + * + * SWC's CommonJS output defines every ESM export as a lazy, non-configurable + * getter, which is why `jest.spyOn(module, 'name')` and + * `jest.replaceProperty(module, 'name', value)` throw "Cannot redefine + * property" on anything SWC transformed. Babel's output defined plain + * properties, so the suite relies on spying on module exports everywhere. + * This wrapper marks those getters `configurable: true` so the spies keep + * working, while leaving the getters lazy (an eager rewrite would break + * modules that import each other in a cycle). + * + * The second difference is what a module in an import cycle sees when it + * reads an export that has not been initialised yet. Babel assigns + * `exports.name = void 0` up front, so the read yields `undefined`; SWC's + * getter returns the live binding and throws a ReferenceError (temporal dead + * zone) instead. Several suites spread `jest.requireActual()` of a module + * that is still mid-evaluation, so the getters are made lenient to match + * Babel: an uninitialised export reads as `undefined`. + */ +// SWC keeps a leading comment of a default export inside the call, as +// `Object.defineProperty(exports, /** ... */ "default", {` or +// `Object.defineProperty(exports, // ...\n"default", {`, hence the optional +// block or line comments before the name. +const COMMENTS = String.raw`(?:(?:\/\*[\s\S]*?\*\/|\/\/[^\n]*)\s*)*`; +const DESCRIPTOR_HEAD = String.raw`Object\.defineProperty\((?:target, name|to, k|exports,\s*${COMMENTS}"[^"]+")`; +const EXPORT_GETTER_DESCRIPTOR = new RegExp( + String.raw`(${DESCRIPTOR_HEAD}, \{\s*enumerable: true,)(\s*)(get:)`, + 'g', +); +// A direct `Object.defineProperty(exports, "name", { enumerable, get` export +// (no `_export(` call), as emitted for single-export modules. +const DIRECT_EXPORT_GETTER = new RegExp( + String.raw`Object\.defineProperty\(exports,\s*${COMMENTS}"(?!__esModule")[^"]+", \{\s*enumerable: true,\s*get\b`, +); +// `_export(exports, { get name () { return binding; }, ... });` is how SWC +// declares named exports; the leniency below is scoped to this block and to +// the `Object.defineProperty(exports, ...)` descriptors, never to getters in +// application code. +const EXPORT_BLOCK = /_export\(exports, \{[\s\S]*?\n\}\);/g; +const EXPORT_BLOCK_GETTER = + /(get [\w$]+ \(\) \{\s*)return ([\w$]+(?:\.[\w$]+)*);/g; Review Comment: A commented default export that sits alongside named exports still isn't made lenient. SWC puts the comment inside the `_export` block as `get // Wrap in memo\n default () { return foo; },`, and this pattern needs `get <name> (`, so that getter is skipped. The fix for the direct `Object.defineProperty(exports, // ...` form doesn't reach it, and the guards don't fire because the other named getters in the block still match. A cyclic import that spreads the partially initialised exports of such a module (WordCloud.tsx, AnnotationLayerControl/index.tsx) would still throw `Cannot access '_default' before initialization`. Could the getter pattern allow the same comment group between `get` and the name? ########## superset-frontend/spec/helpers/swcJestTransformer.test.js: ########## @@ -0,0 +1,211 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { mirrorBabelExportSemantics } from './swcJestTransformer'; + +// The export-helper text is interpolated so this file is not itself rewritten +// by the transformer when Jest loads it; the fixtures stay raw SWC-shaped input. +const EXPORT_FN = '_export'; +const DEFINE = 'defineProperty'; +const GET = 'get'; +const helper = `function ${EXPORT_FN}(target, all) { + for(var name in all)Object.${DEFINE}(target, name, { + enumerable: true, + ${GET}: Object.getOwnPropertyDescriptor(all, name).get + }); +}`; +const exportsBlock = body => `${EXPORT_FN}(exports, { +${body} +});`; + +const NAMED_EXPORTS = `"use strict"; +Object.${DEFINE}(exports, "__esModule", { + value: true +}); +${helper} +${exportsBlock(` ${GET} answer () { + return answer; + }`)} +const answer = 42; +`; + +function load(code) { + const mod = { exports: {} }; + // eslint-disable-next-line no-new-func + new Function('module', 'exports', code)(mod, mod.exports); + return mod.exports; +} + +test('named exports become configurable so jest.spyOn works', () => { + const exports = load(mirrorBabelExportSemantics(NAMED_EXPORTS)); + expect(Object.getOwnPropertyDescriptor(exports, 'answer')).toMatchObject({ + configurable: true, + }); + expect(exports.answer).toBe(42); +}); + +test('an uninitialised export reads as undefined instead of throwing', () => { + const code = NAMED_EXPORTS.replace( + 'const answer = 42;', + 'globalThis.__read = exports.answer;\nlet answer = 42;', + ); + load(mirrorBabelExportSemantics(code)); + expect(globalThis.__read).toBeUndefined(); + delete globalThis.__read; +}); + +test('throws when an export shape is not recognised', () => { + const unknown = `_export(foo, {}); +Object.defineProperty(foo, "x", { + enumerable: true, + get: function() { return 1; } +});`; + expect(() => mirrorBabelExportSemantics(unknown)).toThrow( + /did not match any known shape/, + ); +}); + +test('throws when named export getters change shape but the helper still matches', () => { + const changed = `${helper} +${exportsBlock(` ${GET} answer() { return answer; }`)} +const answer = 42;`; + expect(() => mirrorBabelExportSemantics(changed)).toThrow( + /named export getters did not match/, + ); +}); + +test('member-expression and $-named getters are wrapped leniently', () => { + const code = `"use strict"; +${helper} +const dep = { bar: 7 }; +${exportsBlock(` ${GET} $value () { + return $value; + }, + ${GET} bar () { + return dep.bar; + }`)} +globalThis.__read = [exports.$value, exports.bar]; +let $value = 1; +`; + const patched = mirrorBabelExportSemantics(code); + expect(patched.match(/try \{ return/g)).toHaveLength(2); + load(patched); + expect(globalThis.__read).toEqual([undefined, 7]); + delete globalThis.__read; +}); + +test('a line-commented default export is configurable and lenient', () => { + const code = `"use strict"; +Object.${DEFINE}(exports, "__esModule", { + value: true +}); +globalThis.__read = exports.default; +Object.${DEFINE}(exports, // Wrap in memo +"default", { + enumerable: true, + ${GET}: function() { + return _default; + } +}); +const _default = 5; +`; + const patched = mirrorBabelExportSemantics(code); + expect(patched).toContain('try { return _default;'); + const exports = load(patched); + expect(globalThis.__read).toBeUndefined(); + delete globalThis.__read; + expect(Object.getOwnPropertyDescriptor(exports, 'default')).toMatchObject({ + configurable: true, + }); + expect(exports.default).toBe(5); +}); + +test('a doc-commented default export is configurable and lenient', () => { + const code = `Object.${DEFINE}(exports, /** doc */ "default", { + enumerable: true, + ${GET}: function() { + return _default; + } +}); +const _default = 5; +`; + const patched = mirrorBabelExportSemantics(code); + expect(patched).toContain('try { return _default;'); + expect( + Object.getOwnPropertyDescriptor(load(patched), 'default'), + ).toMatchObject({ configurable: true }); +}); + +test('a from[k] re-export is configurable and lenient', () => { Review Comment: Two of the behaviours this wrapper promises have no failing test. Deleting `if (!(e instanceof ReferenceError)) throw e;` leaves every assertion green, so a real error thrown by a re-exported getter would be silently turned into `undefined`. Likewise, swapping the scoped `EXPORT_BLOCK` rewrite for a file-wide `EXPORT_BLOCK_GETTER` replace would also pass, even though it would rewrite ordinary app getters like `get value () { return later; }`. Separately, the line-commented default test reads `exports.default` before the getter is defined, so its `toBeUndefined()` holds without any leniency. Could you add a re-export fixture whose source getter throws a `TypeError` and assert it propagates, a fixture with an unrelated getter after an export block that still throws its `ReferenceError`, and move the early read after the descriptor is installed? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
