You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/10/22 01:18:23 UTC
[PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
rusackas opened a new pull request, #45595:
URL: https://github.com/apache/superset/pull/45595
### SUMMARY
Webpack moved to SWC in #35946, but Jest still ran every file through Babel. This swaps the Jest transform to `@swc/jest`, with options mirroring `babel.config.js` for the test environment (automatic JSX runtime via `@emotion/react`, emotion labels on so css-in-js snapshots keep their class names, `jest.mock` hoisting, loose class output).
#44210 noted `@swc/jest` wasn't a drop-in because `jest.spyOn` on module exports breaks. The reason is that SWC emits exports as lazy, non-configurable getters, and reads an export that hasn't initialised yet as a ReferenceError, while Babel emitted plain properties pre-set to `undefined`. The suite leans on both Babel behaviours in a few hundred places (`jest.spyOn(module, 'name')`, and spreading `jest.requireActual()` of a module that's mid-evaluation in an import cycle). Rather than rewrite all of those, `spec/helpers/swcJestTransformer.js` wraps `@swc/jest` and post-processes its output to mark the export getters `configurable` and make an uninitialised export read as `undefined`, which is exactly what Babel did. The maintained plugin for this (`@swc-contrib/mut-cjs-exports`) rewrites exports eagerly instead and breaks the chart registry's import cycle, so it wasn't an option.
Four tests still needed edits: `Login.test.tsx` and `apiResources.test.ts` had mock factories referencing a test-local constant that `babel-plugin-jest-hoist` used to hoist for them, `exploreUtils.test.tsx` used `jest.replaceProperty` on what is now an accessor, and `dashboardFilters.test.ts` assigned straight to a module export.
Measured on the 46 SqlLab suites (475 tests) with 2 workers, cold cache then warm:
| transform | cold | warm |
|---|---|---|
| babel-jest | 65 s | 32 s |
| @swc/jest | 39 s | 30 s |
So the win is on cold runs (CI shards, fresh worktrees, anything after a dependency change), where transpiling was about half the wall time. Warm runs are unchanged since test execution dominates there.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TESTING INSTRUCTIONS
CI's sharded Jest job is the real test. Locally, `cd superset-frontend && npm run test -- src/SqlLab` should pass and finish noticeably faster on a cold cache (`npx jest --clearCache` first).
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4391974445
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,153 @@
+/**
Review Comment:
I'm not sure how to review this file, tbh. It almost leans over to the realm of Babel/Webpack/Rolldown, i.e. code transformations. We can treat this as a blackbox and go for the results but IMO, I don't think we can make changes to this code in the future without certain help from Artificial Intelligence.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4373754504
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,135 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return ([\w$]+(?:\.[\w$]+)*|from\[k\]);/g;
Review Comment:
Done in 6dcb5d4: the optional comment group now accepts `//` line comments as well as `/* */`, and the no-match guard also covers the direct `Object.defineProperty(exports, ...)` form. Added fixtures for the line-commented default export.
##########
superset-frontend/spec/helpers/swcJestTransformer.test.js:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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));
Review Comment:
Added fixtures for a doc-commented `"default"` descriptor and a `from[k]` re-export (both assert configurable and lenient), plus a `createTransformer` test covering sync/async output and the cache salt.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4395320456
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,153 @@
+/**
Review Comment:
Human me here... in other words, I'm fine closing this if 30 seconds build time on a cold cache isn't worth the complexity. Think we should close it or keep it? I'm on the fence.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4365685947
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,102 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+const lenient = binding =>
+ `try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
+
+function mirrorBabelExportSemantics(code) {
+ return code
+ .replace(EXPORT_DESCRIPTOR_GETTER, (_m, head, getter, binding) =>
+ `${head}${getter}${lenient(binding)}`,
+ )
+ .replace(EXPORT_BLOCK, block =>
+ block.replace(
+ EXPORT_BLOCK_GETTER,
+ (_m, head, binding) => `${head}${lenient(binding)}`,
+ ),
+ )
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1$2configurable: true,$2$3');
Review Comment:
This replacement inserts a new line into every matched export descriptor (`$2` is the newline plus indentation), but `patch()` returns SWC's source map untouched, including the inline map `@swc/jest` emits by default. Everything after the export preamble then maps one line off per export, so stack traces for a failing test point at the wrong source line and coverage collected under `--coverage` is attributed to the wrong lines. Could the `configurable: true` be inserted on the same line (e.g. `$1 configurable: true,$2$3` without adding a newline), and the lenient `try/catch` wrap likewise kept single-line, so the existing map stays valid?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4358080004
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,87 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `get name () { return binding; }` inside `_export(exports, {...})`, and the
+// `get: function() { return binding; }` form SWC uses for a single export.
+const EXPORT_GETTER_BODY =
+ /(get(?: \w+ \(\)|: function\(\)) \{\s*)return (\w+|from\[k\]);/g;
+
+function mirrorBabelExportSemantics(code) {
+ return code
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1$2configurable: true,$2$3')
+ .replace(
+ EXPORT_GETTER_BODY,
+ '$1try { return $2; } catch (e) { return undefined; }',
Review Comment:
Good catch. The rewrite is now limited to the `_export(exports, ...)` block and the `Object.defineProperty(exports, ...)` descriptors, and it only swallows `ReferenceError`; any other error rethrows. Getters in app code are untouched.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4367606881
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,103 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+const lenient = binding =>
+ `try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
Review Comment:
Added an istanbul ignore hint on the injected try/catch so it no longer reports uncovered branches.
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,103 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+const lenient = binding =>
+ `try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
+
+function mirrorBabelExportSemantics(code) {
+ return code
+ .replace(
+ EXPORT_DESCRIPTOR_GETTER,
+ (_m, head, getter, binding) => `${head}${getter}${lenient(binding)}`,
+ )
+ .replace(EXPORT_BLOCK, block =>
+ block.replace(
+ EXPORT_BLOCK_GETTER,
+ (_m, head, binding) => `${head}${lenient(binding)}`,
+ ),
+ )
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1 configurable: true,$2$3');
+}
+
+function patch(result) {
+ if (typeof result === 'string') return mirrorBabelExportSemantics(result);
+ return { ...result, code: mirrorBabelExportSemantics(result.code) };
+}
+
+const withPatch =
+ fn =>
+ (...args) => {
+ const result = fn(...args);
+ return result && typeof result.then === 'function'
+ ? result.then(patch)
+ : patch(result);
+ };
+
+// Bump when the post-processing changes so cached transforms are invalidated.
+const CACHE_SALT = 'babel-export-semantics-v4';
Review Comment:
Added a small test that runs sample SWC output through the transformer, and the transform now throws if an export block is present but nothing got rewritten.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4381138189
##########
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:
Done in 5eb03c22ad: the named-export getter pattern now allows the same comment group between `get` and the name, with a test for a commented default beside named exports.
##########
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:
Added all three in 5eb03c22ad: a `TypeError` from a re-exported getter propagates, an ordinary getter outside the export block still throws its `ReferenceError`, and the early read now happens 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45595:
URL: https://github.com/apache/superset/pull/45595#issuecomment-6330703999
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge; no actionable issues were identified in the review.
**Assessed commit:** `c4e0f069db65`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4366637131
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,102 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+const lenient = binding =>
+ `try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
+
+function mirrorBabelExportSemantics(code) {
+ return code
+ .replace(EXPORT_DESCRIPTOR_GETTER, (_m, head, getter, binding) =>
+ `${head}${getter}${lenient(binding)}`,
+ )
+ .replace(EXPORT_BLOCK, block =>
+ block.replace(
+ EXPORT_BLOCK_GETTER,
+ (_m, head, binding) => `${head}${lenient(binding)}`,
+ ),
+ )
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1$2configurable: true,$2$3');
Review Comment:
Good catch, @sadpandajoe. `configurable: true` is now inserted on the same line (the lenient `try/catch` was already single-line), so the inline map stays aligned. Cache salt bumped too.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45595:
URL: https://github.com/apache/superset/pull/45595#issuecomment-6330373456
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 6142a7f88539569943c59b0bc58695db83b805a1 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ad96456163999d8f3755e17 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-45595--superset-docs-preview.netlify.app](https://deploy-preview-45595--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AspMz8otS9bNSi0t0i8uTdUuA4sVp-UW5QDUpqQU5-ZWeKUBVZokplmYmpmaGZsaWlpYpFmnG5qamqYbmSrUA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45595--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4372283142
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,135 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return ([\w$]+(?:\.[\w$]+)*|from\[k\]);/g;
Review Comment:
A default export preceded by a `//` comment is not matched here, only a `/* */` one is. SWC emits e.g. `SuperChart.tsx` (`// Wrap in memoβ¦` above `export default memo(SuperChart)`) as `Object.defineProperty(exports, // Wrap in memoβ¦\n"default", {`, so neither this pattern nor `EXPORT_GETTER_DESCRIPTOR` rewrites it. That export stays non-configurable and non-lenient, so `jest.spyOn(module, 'default')` throws `Cannot redefine property: default` and a cyclic read can still hit the ReferenceError. The no-match guards don't fire either, since these single-export modules have no `_export(` call. Could the optional comment group accept line comments too, and could the guard also cover the direct `Object.defineProperty(exports, ...)` form?
##########
superset-frontend/spec/helpers/swcJestTransformer.test.js:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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));
Review Comment:
All five tests build their fixtures from `_export(exports, { get β¦ })`, so `EXPORT_DESCRIPTOR_GETTER` (the `get: function() { return x; }` form used for default exports and `from[k]` re-exports) is never exercised. Deleting that `.replace` leaves this file green while a default export or re-export in a cycle would throw again. Could you add a fixture with a doc-commented `"default"` descriptor and a `Object.defineProperty(to, k, β¦ from[k])` re-export, then assert it is configurable and reads `undefined` before initialisation? `createTransformer` (async path, string vs object result, cache salt) is likewise untested.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #45595:
URL: https://github.com/apache/superset/pull/45595#issuecomment-6487919920
@sadpandajoe @hainenber I'm inclined to close this out. Saving 30s on a local build is nice, but not sure it's worth the complexity here. LMK if you think it's worth keeping, or have any ideas on a better approach.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4352503541
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,87 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `get name () { return binding; }` inside `_export(exports, {...})`, and the
+// `get: function() { return binding; }` form SWC uses for a single export.
+const EXPORT_GETTER_BODY =
+ /(get(?: \w+ \(\)|: function\(\)) \{\s*)return (\w+|from\[k\]);/g;
+
+function mirrorBabelExportSemantics(code) {
+ return code
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1$2configurable: true,$2$3')
+ .replace(
+ EXPORT_GETTER_BODY,
+ '$1try { return $2; } catch (e) { return undefined; }',
Review Comment:
This replacement runs over the whole generated file and catches every exception, so it also rewrites ordinary object getters (e.g. `get value() { return later; }`) and re-export getters (`from[k]`). A getter that throws a genuine error, or reads a real TDZ binding in application code, now silently returns `undefined` under Jest while production still throws, so a test could pass on a bug. Could the rewrite be limited to the `_export(exports, {...})`/`Object.defineProperty(exports, ...)` descriptors and only swallow `ReferenceError`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45595:
URL: https://github.com/apache/superset/pull/45595#issuecomment-6330581986
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45595?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 74.92%. Comparing base ([`2304d1b`](https://app.codecov.io/gh/apache/superset/commit/2304d1b80f2095d9156614e109cb50fb95938ede?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c4e0f06`](https://app.codecov.io/gh/apache/superset/commit/c4e0f069db652272c35f7fa404a0ba6887c7e9d6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
:x: Your project check has failed because the head coverage (99.69%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45595 +/- ##
==========================================
- Coverage 84.10% 74.92% -9.19%
==========================================
Files 3056 3243 +187
Lines 201276 223633 +22357
Branches 47064 54769 +7705
==========================================
- Hits 169289 167560 -1729
- Misses 28747 52833 +24086
Partials 3240 3240
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45595/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/45595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.98% <ΓΈ> (-17.18%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45595?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4382433752
##########
superset-frontend/spec/helpers/swcJestTransformer.test.js:
##########
@@ -0,0 +1,280 @@
+/**
+ * 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
+});
+Object.${DEFINE}(exports, // Wrap in memo
+"default", {
+ enumerable: true,
+ ${GET}: function() {
+ return _default;
+ }
+});
+globalThis.__read = exports.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 commented default export beside named exports is lenient', () => {
+ const code = `${helper}
+${exportsBlock(` ${GET} answer () {
+ return answer;
+ },
+ ${GET} // Wrap in memo
+ default () {
+ return _default;
+ }`)}
+globalThis.__read = [exports.answer, exports.default];
+let answer = 1;
+const _default = 5;
+`;
+ const patched = mirrorBabelExportSemantics(code);
+ expect(patched.match(/try \{ return/g)).toHaveLength(2);
+ const exports = load(patched);
+ expect(globalThis.__read).toEqual([undefined, undefined]);
+ delete globalThis.__read;
+ expect(exports.default).toBe(5);
+});
+
+test('a non-ReferenceError from a re-exported getter propagates', () => {
+ const code = `function _exportStar(from, to) {
+ Object.keys(from).forEach(function(k) {
+ if (k !== "default") Object.${DEFINE}(to, k, {
+ enumerable: true,
+ ${GET}: function() {
+ return from[k];
+ }
+ });
+ });
+ return from;
+}
+const source = {};
+Object.${DEFINE}(source, "boom", {
+ enumerable: true,
+ ${GET}() { throw new TypeError("boom"); }
+});
+_exportStar(source, exports);
+`;
+ const exports = load(mirrorBabelExportSemantics(code));
+ expect(() => exports.boom).toThrow(TypeError);
+});
+
+test('getters outside the export block keep throwing their ReferenceError', () => {
+ const code = `${helper}
+${exportsBlock(` ${GET} answer () {
+ return answer;
+ }`)}
+const answer = 1;
+const obj = {
+ ${GET} value () {
+ return later;
+ }
+};
+try {
+ obj.value;
+} catch (e) {
+ globalThis.__early = e.name;
+}
+let later = 2;
+`;
+ const patched = mirrorBabelExportSemantics(code);
+ expect(patched.match(/try \{ return/g)).toHaveLength(1);
+ load(patched);
+ expect(globalThis.__early).toBe('ReferenceError');
+ delete globalThis.__early;
+});
+
+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', () => {
+ const code = `function _exportStar(from, to) {
+ Object.keys(from).forEach(function(k) {
+ if (k !== "default") Object.${DEFINE}(to, k, {
+ enumerable: true,
+ ${GET}: function() {
+ return from[k];
+ }
+ });
+ });
+ return from;
+}
+_exportStar({ a: 1 }, exports);
+`;
+ const patched = mirrorBabelExportSemantics(code);
+ expect(patched).toContain('try { return from[k];');
+ const exports = load(patched);
+ expect(Object.getOwnPropertyDescriptor(exports, 'a')).toMatchObject({
+ configurable: true,
+ });
+ expect(exports.a).toBe(1);
+});
+
+test('throws on an unrecognised direct defineProperty export', () => {
+ const unknown = `Object.${DEFINE}(exports, "x", {
+ enumerable: true,
+ ${GET}() { return 1; }
+});`;
+ expect(() => mirrorBabelExportSemantics(unknown)).toThrow(
+ /did not match any known shape/,
+ );
+});
+
+test('createTransformer patches sync and async output and salts the cache key', async () => {
+ const raw = `${helper}
+${exportsBlock(` ${GET} answer () {
+ return answer;
+ }`)}
+const answer = 1;
+`;
+ const swc = {
+ process: jest.fn(() => ({ code: raw })),
+ processAsync: jest.fn(async () => raw),
+ getCacheKey: jest.fn(() => 'base'),
+ };
+ jest.doMock('@swc/jest', () => ({ createTransformer: () => swc }));
+ let transformer;
+ jest.isolateModules(() => {
+ // eslint-disable-next-line global-require
+ transformer = require('./swcJestTransformer').createTransformer({});
+ });
+ jest.dontMock('@swc/jest');
+ expect(transformer.process().code).toContain('configurable: true');
Review Comment:
The wrapper test only feeds `process` a bare `{ code }` result and `processAsync` a bare string, so nothing checks that the rest of SWC's result survives the patch. Changing `patch` to return `{ code: mirrorBabelExportSemantics(result.code) }` (dropping `...result`), or leaving an async object result unpatched, keeps every assertion green, yet the inline source map is what keeps stack traces and coverage mapped to the original lines (the line-alignment concern earlier in this review). Could the mocked `process`/`processAsync` return `{ code: raw, map: sentinel }` and assert that the map comes back unchanged and that the async object's `code` is rewritten too?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on PR #45595:
URL: https://github.com/apache/superset/pull/45595#issuecomment-6488034861
Im good with closing this out. Long term, Vitest is the way to go, even with jsdom
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4368933247
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,123 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+// The istanbul hint keeps the injected try/catch out of coverage instrumentation
+// so exported bindings do not report uncovered branches.
+const lenient = binding =>
+ `/* istanbul ignore next */ try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
+
+function rewriteExports(code) {
+ return code
+ .replace(
+ EXPORT_DESCRIPTOR_GETTER,
+ (_m, head, getter, binding) => `${head}${getter}${lenient(binding)}`,
+ )
+ .replace(EXPORT_BLOCK, block =>
+ block.replace(
+ EXPORT_BLOCK_GETTER,
+ (_m, head, binding) => `${head}${lenient(binding)}`,
+ ),
+ )
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1 configurable: true,$2$3');
+}
+
+export function mirrorBabelExportSemantics(code) {
+ const patched = rewriteExports(code);
+ // A shape change in SWC's export helpers would otherwise leave exports
+ // non-configurable, surfacing later as scattered "Cannot redefine property"
+ // errors or TDZ ReferenceErrors in unrelated suites.
+ if (
+ /\b_export\(/.test(code) &&
+ /enumerable: true,\s*get\b/.test(code) &&
+ !patched.includes('configurable: true')
Review Comment:
This check passes whenever any `configurable: true` is present, but the `_export` helper descriptor always supplies one. If SWC changes only the named-getter shape (say `get answer() {` instead of `get answer () {`), `EXPORT_BLOCK_GETTER` stops matching, the helper is still patched, and the guard stays quiet. The leniency half then silently disappears and uninitialised exports throw `ReferenceError` again in the cyclic-import suites, far from the cause. Should the guard also require that a `try { return` rewrite happened whenever the code contains a getter body, and should the test cover a fixture where only the getter shape differs?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4371604377
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,134 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+// The istanbul hint keeps the injected try/catch out of coverage instrumentation
+// so exported bindings do not report uncovered branches.
+const lenient = binding =>
+ `/* istanbul ignore next */ try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
+
+function rewriteExports(code) {
+ return code
+ .replace(
+ EXPORT_DESCRIPTOR_GETTER,
+ (_m, head, getter, binding) => `${head}${getter}${lenient(binding)}`,
+ )
+ .replace(EXPORT_BLOCK, block =>
+ block.replace(
+ EXPORT_BLOCK_GETTER,
+ (_m, head, binding) => `${head}${lenient(binding)}`,
+ ),
+ )
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1 configurable: true,$2$3');
+}
+
+export function mirrorBabelExportSemantics(code) {
+ const patched = rewriteExports(code);
+ // A shape change in SWC's export helpers would otherwise leave exports
+ // non-configurable, surfacing later as scattered "Cannot redefine property"
+ // errors or TDZ ReferenceErrors in unrelated suites.
+ if (
+ /\b_export\(/.test(code) &&
+ /enumerable: true,\s*get\b/.test(code) &&
+ !patched.includes('configurable: true')
+ ) {
+ throw new Error(
+ 'swcJestTransformer: SWC export output did not match any known shape; ' +
+ 'update the patterns in spec/helpers/swcJestTransformer.js',
+ );
+ }
+ // The helper descriptor always supplies `configurable: true`, so a changed
+ // named-getter shape would otherwise lose only the leniency half unnoticed.
+ if (
+ /\b_export\(exports, \{[\s\S]*?\bget\s+\w+\s*\(/.test(code) &&
Review Comment:
This new guard throws on any module whose `_export` getters return a member expression. A barrel like `packages/superset-ui-core/src/index.ts` re-exports with getters such as `return _foo.bar;`, but `EXPORT_BLOCK_GETTER` only rewrites a bare `\w+` binding, so nothing gets a `try { return` and the guard throws "named export getters did not match". The failing `sharded-jest-tests` shards at this head show it (e.g. `src/utils/downloadAsImage.test.ts` fails to run, with the stack going through `superset-ui-core/src/index.ts`), so every suite that imports that package fails to load. The same regex also skips `$`-containing names, so a block mixing `$value` and a plain name would pass the guard while leaving `$value` unwrapped. Should the getter pattern accept member-expression and `$` identifiers (or the guard check only the shapes the rewrite actually supports)?
##########
superset-frontend/spec/helpers/swcJestTransformer.test.js:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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';
+
+const NAMED_EXPORTS = `"use strict";
+Object.defineProperty(exports, "__esModule", {
+ value: true
+});
+function _export(target, all) {
+ for(var name in all)Object.defineProperty(target, name, {
+ enumerable: true,
+ get: Object.getOwnPropertyDescriptor(all, name).get
+ });
+}
+_export(exports, {
+ 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));
Review Comment:
This test file goes through the same transformer when Jest loads it, and the `NAMED_EXPORTS` template literal contains `_export(exports, {...})` and the helper's `Object.defineProperty(target, name, {` text, so the fixture is already rewritten (configurable, `try` getter) before `mirrorBabelExportSemantics` is called on it here. That means the configurable and uninitialised-export assertions at lines 48-51 and 54-62 still pass if `mirrorBabelExportSemantics` becomes a no-op, so a regression in the rewrite would not fail them. Could the fixture be built so the transformer cannot see the export-helper text (as the last test already does for `get`), so these tests exercise raw SWC-shaped input?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4385078413
##########
superset-frontend/spec/helpers/swcJestTransformer.test.js:
##########
@@ -0,0 +1,280 @@
+/**
+ * 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
+});
+Object.${DEFINE}(exports, // Wrap in memo
+"default", {
+ enumerable: true,
+ ${GET}: function() {
+ return _default;
+ }
+});
+globalThis.__read = exports.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 commented default export beside named exports is lenient', () => {
+ const code = `${helper}
+${exportsBlock(` ${GET} answer () {
+ return answer;
+ },
+ ${GET} // Wrap in memo
+ default () {
+ return _default;
+ }`)}
+globalThis.__read = [exports.answer, exports.default];
+let answer = 1;
+const _default = 5;
+`;
+ const patched = mirrorBabelExportSemantics(code);
+ expect(patched.match(/try \{ return/g)).toHaveLength(2);
+ const exports = load(patched);
+ expect(globalThis.__read).toEqual([undefined, undefined]);
+ delete globalThis.__read;
+ expect(exports.default).toBe(5);
+});
+
+test('a non-ReferenceError from a re-exported getter propagates', () => {
+ const code = `function _exportStar(from, to) {
+ Object.keys(from).forEach(function(k) {
+ if (k !== "default") Object.${DEFINE}(to, k, {
+ enumerable: true,
+ ${GET}: function() {
+ return from[k];
+ }
+ });
+ });
+ return from;
+}
+const source = {};
+Object.${DEFINE}(source, "boom", {
+ enumerable: true,
+ ${GET}() { throw new TypeError("boom"); }
+});
+_exportStar(source, exports);
+`;
+ const exports = load(mirrorBabelExportSemantics(code));
+ expect(() => exports.boom).toThrow(TypeError);
+});
+
+test('getters outside the export block keep throwing their ReferenceError', () => {
+ const code = `${helper}
+${exportsBlock(` ${GET} answer () {
+ return answer;
+ }`)}
+const answer = 1;
+const obj = {
+ ${GET} value () {
+ return later;
+ }
+};
+try {
+ obj.value;
+} catch (e) {
+ globalThis.__early = e.name;
+}
+let later = 2;
+`;
+ const patched = mirrorBabelExportSemantics(code);
+ expect(patched.match(/try \{ return/g)).toHaveLength(1);
+ load(patched);
+ expect(globalThis.__early).toBe('ReferenceError');
+ delete globalThis.__early;
+});
+
+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', () => {
+ const code = `function _exportStar(from, to) {
+ Object.keys(from).forEach(function(k) {
+ if (k !== "default") Object.${DEFINE}(to, k, {
+ enumerable: true,
+ ${GET}: function() {
+ return from[k];
+ }
+ });
+ });
+ return from;
+}
+_exportStar({ a: 1 }, exports);
+`;
+ const patched = mirrorBabelExportSemantics(code);
+ expect(patched).toContain('try { return from[k];');
+ const exports = load(patched);
+ expect(Object.getOwnPropertyDescriptor(exports, 'a')).toMatchObject({
+ configurable: true,
+ });
+ expect(exports.a).toBe(1);
+});
+
+test('throws on an unrecognised direct defineProperty export', () => {
+ const unknown = `Object.${DEFINE}(exports, "x", {
+ enumerable: true,
+ ${GET}() { return 1; }
+});`;
+ expect(() => mirrorBabelExportSemantics(unknown)).toThrow(
+ /did not match any known shape/,
+ );
+});
+
+test('createTransformer patches sync and async output and salts the cache key', async () => {
+ const raw = `${helper}
+${exportsBlock(` ${GET} answer () {
+ return answer;
+ }`)}
+const answer = 1;
+`;
+ const swc = {
+ process: jest.fn(() => ({ code: raw })),
+ processAsync: jest.fn(async () => raw),
+ getCacheKey: jest.fn(() => 'base'),
+ };
+ jest.doMock('@swc/jest', () => ({ createTransformer: () => swc }));
+ let transformer;
+ jest.isolateModules(() => {
+ // eslint-disable-next-line global-require
+ transformer = require('./swcJestTransformer').createTransformer({});
+ });
+ jest.dontMock('@swc/jest');
+ expect(transformer.process().code).toContain('configurable: true');
Review Comment:
Done in eaae59a479: the mocked `process`/`processAsync` now return `{ code, map }`, and the test asserts the map comes back unchanged and the async object `code` is rewritten.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4367085216
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,103 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+const lenient = binding =>
+ `try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
Review Comment:
Every export getter now carries a `try/catch` with an `if (!(e instanceof ReferenceError)) throw e` that normal execution never enters. Coverage instrumentation runs on the transformed output and maps these injected branches back onto the original export declaration, so each exported binding picks up uncovered branches (a one-line `export const answer = 42` reports 100% lines/statements but 0% branches). That would skew Codecov branch numbers across the tree and trip the 100% branch threshold in `npm run core:cover` for `packages/**`. Could the injected code be excluded from instrumentation (for example with an `/* istanbul ignore next */` hint on the generated catch), or have you confirmed coverage output is unchanged?
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,103 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+const lenient = binding =>
+ `try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
+
+function mirrorBabelExportSemantics(code) {
+ return code
+ .replace(
+ EXPORT_DESCRIPTOR_GETTER,
+ (_m, head, getter, binding) => `${head}${getter}${lenient(binding)}`,
+ )
+ .replace(EXPORT_BLOCK, block =>
+ block.replace(
+ EXPORT_BLOCK_GETTER,
+ (_m, head, binding) => `${head}${lenient(binding)}`,
+ ),
+ )
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1 configurable: true,$2$3');
+}
+
+function patch(result) {
+ if (typeof result === 'string') return mirrorBabelExportSemantics(result);
+ return { ...result, code: mirrorBabelExportSemantics(result.code) };
+}
+
+const withPatch =
+ fn =>
+ (...args) => {
+ const result = fn(...args);
+ return result && typeof result.then === 'function'
+ ? result.then(patch)
+ : patch(result);
+ };
+
+// Bump when the post-processing changes so cached transforms are invalidated.
+const CACHE_SALT = 'babel-export-semantics-v4';
Review Comment:
The three regexes depend on SWC's exact helper shape (`_export(exports, {...})`, the `target, name` / `to, k` parameter names, `get: function() {`), but nothing fails when none of them match. After an `@swc/core` or `@swc/jest` bump that renames those helpers, the transform still succeeds with no rewrite, and the symptom only surfaces as scattered "Cannot redefine property" errors or reappearing TDZ `ReferenceError`s in unrelated suites. Would a small test that runs a sample module through this transformer and asserts `configurable: true` plus an uninitialised-export read returning `undefined` (or a throw in `process` when a module with exports produced zero replacements) be feasible, so a shape change is caught at the source?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4371537454
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,123 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+// The istanbul hint keeps the injected try/catch out of coverage instrumentation
+// so exported bindings do not report uncovered branches.
+const lenient = binding =>
+ `/* istanbul ignore next */ try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
+
+function rewriteExports(code) {
+ return code
+ .replace(
+ EXPORT_DESCRIPTOR_GETTER,
+ (_m, head, getter, binding) => `${head}${getter}${lenient(binding)}`,
+ )
+ .replace(EXPORT_BLOCK, block =>
+ block.replace(
+ EXPORT_BLOCK_GETTER,
+ (_m, head, binding) => `${head}${lenient(binding)}`,
+ ),
+ )
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1 configurable: true,$2$3');
+}
+
+export function mirrorBabelExportSemantics(code) {
+ const patched = rewriteExports(code);
+ // A shape change in SWC's export helpers would otherwise leave exports
+ // non-configurable, surfacing later as scattered "Cannot redefine property"
+ // errors or TDZ ReferenceErrors in unrelated suites.
+ if (
+ /\b_export\(/.test(code) &&
+ /enumerable: true,\s*get\b/.test(code) &&
+ !patched.includes('configurable: true')
Review Comment:
Good catch, @sadpandajoe. The guard now also throws when an `_export` block has a named getter but no `try { return` rewrite happened, and a test covers a `get answer()` shape change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4372188528
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,134 @@
+/**
+ * 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 doc comment of a default export inside the call, as
+// `Object.defineProperty(exports, /** ... */ "default", {`, hence the optional
+// block comment before the name.
+const EXPORT_GETTER_DESCRIPTOR =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,)(\s*)(get:)/g;
+// `_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+);/g;
+// The `get: function() { return binding; }` form SWC uses for a single export
+// or a re-export (`from[k]`), matched together with its descriptor.
+const EXPORT_DESCRIPTOR_GETTER =
+ /(Object\.defineProperty\((?:target, name|to, k|exports,\s*(?:\/\*[\s\S]*?\*\/\s*)?"[^"]+"), \{\s*enumerable: true,\s*)(get: function\(\) \{\s*)return (\w+|from\[k\]);/g;
+
+// Only a TDZ ReferenceError is swallowed; any other error still propagates.
+// The istanbul hint keeps the injected try/catch out of coverage instrumentation
+// so exported bindings do not report uncovered branches.
+const lenient = binding =>
+ `/* istanbul ignore next */ try { return ${binding}; } catch (e) { if (!(e instanceof ReferenceError)) throw e; return undefined; }`;
+
+function rewriteExports(code) {
+ return code
+ .replace(
+ EXPORT_DESCRIPTOR_GETTER,
+ (_m, head, getter, binding) => `${head}${getter}${lenient(binding)}`,
+ )
+ .replace(EXPORT_BLOCK, block =>
+ block.replace(
+ EXPORT_BLOCK_GETTER,
+ (_m, head, binding) => `${head}${lenient(binding)}`,
+ ),
+ )
+ .replace(EXPORT_GETTER_DESCRIPTOR, '$1 configurable: true,$2$3');
+}
+
+export function mirrorBabelExportSemantics(code) {
+ const patched = rewriteExports(code);
+ // A shape change in SWC's export helpers would otherwise leave exports
+ // non-configurable, surfacing later as scattered "Cannot redefine property"
+ // errors or TDZ ReferenceErrors in unrelated suites.
+ if (
+ /\b_export\(/.test(code) &&
+ /enumerable: true,\s*get\b/.test(code) &&
+ !patched.includes('configurable: true')
+ ) {
+ throw new Error(
+ 'swcJestTransformer: SWC export output did not match any known shape; ' +
+ 'update the patterns in spec/helpers/swcJestTransformer.js',
+ );
+ }
+ // The helper descriptor always supplies `configurable: true`, so a changed
+ // named-getter shape would otherwise lose only the leniency half unnoticed.
+ if (
+ /\b_export\(exports, \{[\s\S]*?\bget\s+\w+\s*\(/.test(code) &&
Review Comment:
Good catch. The getter patterns now accept `$` identifiers and dotted member expressions like `_foo.bar`, so barrel re-exports are wrapped instead of tripping the guard. Added a test covering both shapes.
##########
superset-frontend/spec/helpers/swcJestTransformer.test.js:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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';
+
+const NAMED_EXPORTS = `"use strict";
+Object.defineProperty(exports, "__esModule", {
+ value: true
+});
+function _export(target, all) {
+ for(var name in all)Object.defineProperty(target, name, {
+ enumerable: true,
+ get: Object.getOwnPropertyDescriptor(all, name).get
+ });
+}
+_export(exports, {
+ 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));
Review Comment:
Right, the fixture was being rewritten by the transformer when Jest loaded the file. The helper and export-block text are now interpolated so the fixtures stay raw SWC-shaped input.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4391979229
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,153 @@
+/**
Review Comment:
Remaining are LGTM though!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(frontend): run Jest through @swc/jest instead of babel-jest [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #45595:
URL: https://github.com/apache/superset/pull/45595#discussion_r4395282146
##########
superset-frontend/spec/helpers/swcJestTransformer.js:
##########
@@ -0,0 +1,153 @@
+/**
Review Comment:
Heya @hainenber, fair point, and I won't pretend it's a fun file to read cold. It's contained to just this one spot though, has a dedicated test suite covering every shape @sadpandajoe found across our back-and-forth, and throws loudly instead of silently reverting to broken behavior if SWC's output ever changes shape. So I think the actual maintenance risk is lower than it looks on first read.
Worth being honest about what it buys too: about 30 seconds on a cold-cache local run, nothing on CI since the sharded time came out the same either way. If that's not meaningful for people running locally, I'm perfectly happy just closing this instead of taking on the complexity. Curious what you both think.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org