Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

commonjs wildcard imports only expose enumerable properties of requirees' exports #1778

Closed
dbjorge opened this issue Jun 2, 2021 · 2 comments
Labels

Comments

@dbjorge
Copy link

dbjorge commented Jun 2, 2021

Describe the bug

_interopRequireWildcard's implementation works by using a for(var key in obj) loop over the required module's export to build up a new object with the same keys. This strategy does not cooperate with modules whose exports expose other types of capabilities. For example, it does not support commonjs modules whose exports are:

  • Proxy objects which support get operations of dynamic, non-enumerable properties (eg, identity-obj-proxy)
  • Classes which support being constructed (eg, enzyme-adapter-react-16)

The specific motivating module in our case is identity-obj-proxy, which the Jest docs suggest as the recommended way to mock CSS modules. Its implementation is essentially:

idObj = new Proxy({}, {
  get: function getter(target, key) {
    if (key === '__esModule') {
      return false;
    }
    return key;
  }
});

module.exports = idObj;

We found this issue while experimenting with migrating an existing React codebase (microsoft/accessibility-insights-web) from ts-jest to @swc/jest. Our project uses identity-obj-proxy per the Jest docs' suggestion of how to mock CSS modules, like this:

// jest.config.js
{
    moduleNameMapper: {
        '\\.(scss)$': 'identity-obj-proxy',
    },
    transform: {
        // works
        // '^.+\\.(ts|tsx)$': 'ts-jest',

        // breaks
         '^.+\\.(ts|tsx)$': ['@swc/jest'],
    },
}
// my-component.tsx
import * as styles from './footer-section.scss';
export const MyComponent = () => {
    return <button className={styles.myCoolButton} />;
};

With ts-jest, this strategy results in the component seeing styles.myCoolButton as "myCoolButton"; with @swc/jest, this results in the component seeing styles.myCoolButton as undefined, because _interopRequireWildcard' doesn't know to add a myCoolButton property to the synthetic export it creates.

The second motivating case we ran into was with the enzyme-adapter-react-16 module, which exports a class and expects you to construct an instance of it. The synthetic export from _interopRequireWildcard does not support having new instances constructed from it.

Input code

repro-proxy-export.ts:

import * as identityObjProxy from 'identity-obj-proxy';
console.log(`identityObjProxy.testVar: ${identityObjProxy.testVar}`);

repro-class-export.ts:

import * as EnzymeAdapter from 'enzyme-adapter-react-16';
new EnzymeAdapter(); // shouldn't throw
console.log('Test passed!');

.swcrc:

{ "module": { "type": "commonjs" } }
> npm install @swc/core @swc/cli identity-obj-proxy enzyme-adapter-react-16 enzyme react@16 react-dom@16
> npx swc -d dist ./repro-class-export.ts ./repro-proxy-export.ts

> # should print identityObjProxy.testVar: testVar
> node ./dist/repro-proxy-export.js
identityObjProxy.testVar: undefined

> # should run without throwing
> node ./dist/repro-class-export.js
C:\repos\swc-repro\dist\repro-class-export.js:26
new EnzymeAdapter(); // shouldn't throw
^

TypeError: EnzymeAdapter is not a constructor
    at Object.<anonymous> (C:\repos\swc-repro\dist\repro-class-export.js:26:1)
    at Module._compile (internal/modules/cjs/loader.js:1068:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1097:10)
    at Module.load (internal/modules/cjs/loader.js:933:32)
    at Function.Module._load (internal/modules/cjs/loader.js:774:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47

Config

With @swc/jest, default configuration (no .swcrc file) repros. For standalone repro:

{ "module": { "type": "commonjs" } }

Expected behavior

The two repros from the "Input code" section above should respectively:

  • print identityObjProxy.testVar: testVar
  • not emit an error

Version

  • @swc/core: 1.2.59
  • @swc/jest: 0.1.2

Additional context

n/a

@dbjorge dbjorge added the C-bug label Jun 2, 2021
@dbjorge
Copy link
Author

dbjorge commented Jun 2, 2021

On closer inspection, I'm not so convinced that this is really a bug that makes sense for swc to fix; tsc exhibits the same behavior if esModuleInterop is set to true, and swc gives the desired/expected behavior if I add noInterop to my .swcrc config.

I think it's pretty reasonable for swc's interop behavior to be consistent with tsc's esModuleInterop behavior, so I'm going to close the bug; I just wish I'd noticed the difference in defaults before spending time investigating :)

For anyone else encountering a similar looking error, the workaround you're probably looking for is:

.swcrc:

{
    "module": {
        "noInterop": true
    }
}

@dbjorge dbjorge closed this as completed Jun 2, 2021
dbjorge added a commit to microsoft/accessibility-insights-web that referenced this issue Jun 4, 2021
#### Details

Profiling our unit tests showed that the overwhelming majority of time spent was in `ts-jest` parsing typescript files. This PR is an experiment in replacing `ts-jest` with an alternative typescript transpiler, `swc`, which is less mature but much faster.

##### Motivation

On my local machine (a Surface Laptop 3 running Windows 21H1 OS Build 19043.985), comparative timings are:

| scenario | `main` (ts-jest) | this PR (swc) |
| - | - | - |
| `yarn test -- -- --coverage=false -- footer-section.test.tsx` | 12s ±1s | 4.3s ±0.2s |
| `yarn test -- -- -- footer-section.test.tsx` | 45s ±3s | 6.2s ±0.0s |
| `yarn test` | 11m54s | 3m ±14s |

On our CI build agents:

| scenario | `main` (ts-jest) | this PR (swc) |
| - | - | - |
| unit tests | [15m1s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21734&view=logs&j=3613c2bd-8b62-5a70-8491-3e8459586450&t=d1343856-9cd9-5649-cdc6-81bc16053e6f) | [5m9s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21738&view=logs&j=3613c2bd-8b62-5a70-8491-3e8459586450&t=d1343856-9cd9-5649-cdc6-81bc16053e6f) | 
| web-e2e tests | [10m10s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21734&view=logs&j=952ed597-794a-5756-0328-a93bb2daa2a4&t=46864a42-71df-5c2d-c60f-5fec8d56499d) | [8m6s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21738&view=logs&j=952ed597-794a-5756-0328-a93bb2daa2a4&t=46864a42-71df-5c2d-c60f-5fec8d56499d) |
| unified-e2e tests | [10m39s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21734&view=logs&j=c5567cdb-c930-534d-6489-7c2dcf7ac8ee&t=fe6db478-348c-54d0-bafe-001288e28283) | [10m33s](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21738&view=logs&j=c5567cdb-c930-534d-6489-7c2dcf7ac8ee&t=fe6db478-348c-54d0-bafe-001288e28283) |
| total PR build time | [~22m](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build?definitionId=38&_a=summary) | [~14m](https://dev.azure.com/accessibility-insights/accessibility-insights-web/_build/results?buildId=21740&view=results) |

##### Context

This is not as clear cut a transition as the timings above suggest. `swc` is a relatively young project; it's not quite as firmly single-maintainer as `esbuild`, but it's still got a fairly concerning bus factor compared to `tsc` or `babel` (what `ts-jest` hands off to). Still, the timings speak for themselves and it's easy enough to transition back if that ever becomes necessary.

If this experiment goes well, we might consider a similar change to update our webpack config to use `swc-loader` instead of `ts-loader`.

One of the reasons `swc` is so much faster is that it does not perform type-checking. Right now we do type-checking as part of build, but if we were to switch webpack to use swc, we'd then need a separate build step for type-checking (comparable to the current `yarn null:check` step).

Implementation notes:
* There are a few subtly important things in the `.swcrc` file, which unfortunately doesn't support comments:
    * The `noInterop: true` flag is the equivalent of TypeScript's default `esModuleInterop: false` behavior. It is necessary; we use several modules which cannot be used correctly with interop behavior in play. See swc-project/swc#1778 for details.
    * The empty `"env": {}` property is necessary to trigger `swc` to respect the `browserslist` entry in `package.json`
    * Directing `swc` to target a reasonably recent set of browsers (via the new `browserslist` entry in `package.json`) is required; there are several tests with snapshots that change under default targeting, and there are a few components that break outright.
    * `tsx: true` is required (it tells `swc` to enable support for `tsx` files; it doesn't prevent non-tsx files from parsing)
* For now, this is using a custom Jest transformer to call `@swc/core`. The swc project maintains a `@swc/jest` transformer, but swc-project/jest#8 prevents it from being able to use the `noInterop: true` setting we require, and I can't submit a PR to patch the project until legal rubber stamps their CLA
* `collapsible-script-provider.tsx` required a hacky workaround for swc-project/swc#1165
    * even after applying the workaround, running jest with and without `--coverage=false` emitted javascript with different whitespace. `collapsible-script-provider.test.ts` needed to be updated to format the resulting js before snapshotting it.
* `src/scanner/processor.ts` used a mutable global in a module to enable its tests to inject test data. This confused swc; I replaced it with a more typical ctor injection pattern.
* `browser-adapter-factory.test.tsx` did a dance around an `import 'webextension-polyfill-ts';` statement to trick the module into thinking `window.chrome.runtime.id` is defined (it checks for this during import and refuses to load if it is not set). The dance we used is not compliant with how ES6 imports are defined to work (they are defined as hoisting import statements to the top of the file, ie, before we stub the window property). TypeScript has an outstanding PR/issue to stop supporting what we were previously doing (microsoft/TypeScript#39764) and swc intentionally doesn't support it (swc-project/swc#1686) (babel doesn't either), so I had to modify how our dance worked by moving it to a separate file and having it use a raw `require()` rather than an `import`.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox -->
- [n/a] Addresses an existing issue: #0000
- [x] Ran `yarn fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See `CONTRIBUTING.md`.
- [n/a] (UI changes only) Added screenshots/GIFs to description above
- [n/a] (UI changes only) Verified usability with NVDA/JAWS
@swc-bot
Copy link
Collaborator

swc-bot commented Oct 24, 2022

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

2 participants