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

fix(webdriverjs): fix default commonJs export #927

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

straker
Copy link
Contributor

@straker straker commented Nov 16, 2023

This fixes the breaking change we accidentally released in 4.7.3 when we broke the default export of @axe-core/webdriverjs. Also added tests to ensure we don't make the same mistake again.

Closes #940

@straker straker requested a review from a team as a code owner November 16, 2023 23:07
@straker straker force-pushed the webdriverjs-default-export branch from 97104ba to 5425097 Compare November 16, 2023 23:25
@stephenmathieson
Copy link
Member

This PR touches a lot more than webdriverjs. May want to update the commit scope 🙂

Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to also include tests of importing from typescript; we've seen at least one case where a CJS/ESM-related misconfiguration resulted in a package that could be used successfully from JS, but resulted in an empty typings file that broke importing from typescript. Maybe something like:

// This file isn't executed; we only care that typescript can successfully
// build/typecheck. It is a smoke test that our build process is producing a
// basically-reasonable index.d.ts.

// Detailed tests of individual types should instead be covered by unit tests.

import { AxeBuilder as NamedImportAxeBuilder } from '../dist/index.js';
import DefaultImportAxeBuilder from '../dist/index.js';
import type { WebDriver } from 'selenium-webdriver';

// See https://stackoverflow.com/a/55541672
type IsAny<T> = 0 extends (1 & T) ? true : false;

// If the imports don't have typings assigned, these will fail
// with "ts(2322): Type 'false' is not assignable to type 'true'."
(x: IsAny<typeof NamedImportAxeBuilder>): false => x;
(x: IsAny<typeof DefaultImportAxeBuilder>): false => x;

new NamedImportAxeBuilder({} as WebDriver)
  .withRules('color-contrast')
  .analyze();

new DefaultImportAxeBuilder({} as WebDriver)
  .withRules('color-contrast')
  .analyze();

packages/webdriverjs/src/index.ts Outdated Show resolved Hide resolved
@@ -291,4 +291,10 @@ export default class AxeBuilder {
}
}

// ensure backwards compatibility with commonJs default export
if (typeof module === 'object') {
module.exports = AxeBuilder;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that this causes some warning spew in the build:

warning spew
/repos/axe-core-npm/packages/webdriverjs (webdriverjs-default-export*) » yarn build                               danbjorge@Dans-MacBook-Pro
yarn run v1.22.19
$ rimraf dist
$ tsup src/index.ts --dts --format esm,cjs
CLI Building entry: src/index.ts
CLI Using tsconfig: tsconfig.json
CLI tsup v6.7.0
CLI Target: esnext
ESM Build start
CJS Build start

 WARN  ▲ [WARNING] The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected [commonjs-variable-in-esm]

    src/index.ts:296:2:
      296 │   module.exports = AxeBuilder
          ╵   ~~~~~~

  This file is considered to be an ECMAScript module because of the "export" keyword here:

    src/index.ts:301:0:
      301 │ export { AxeBuilder };
          ╵ ~~~~~~




 WARN  ▲ [WARNING] The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected [commonjs-variable-in-esm]

    src/index.ts:296:2:
      296 │   module.exports = AxeBuilder
          ╵   ~~~~~~

  This file is considered to be an ECMAScript module because of the "export" keyword here:

    src/index.ts:301:0:
      301 │ export { AxeBuilder };
          ╵ ~~~~~~



ESM dist/index.mjs 14.24 KB
ESM ⚡️ Build success in 39ms
CJS dist/index.js 15.97 KB
CJS ⚡️ Build success in 39ms
DTS Build start
^[[A^[[ADTS ⚡️ Build success in 809ms
DTS dist/index.d.ts 2.52 KB
✨  Done in 1.27s.

This is a reasonable warning, but one that we reasonably want to suppress here, I think. I didn't see a good way of doing that - it comes from esbuild (not eslint), which doesn't support a suppress-next-line type comment. esbuild does have a --log-override option to quiet it, but only for the whole build process, not for a few lines, and tsup doesn't support forwarding it anyway (they tried but encountered some issues getting it to work and abandoned it :( )

I don't think it's worth more time trying to suppress the warning, just noting it here so noone repeats the same research.

@straker
Copy link
Contributor Author

straker commented Nov 20, 2023

I'm going to split the TS type check and the support for default/.default/named export into their own prs, though I will add the default/.default/named export into webdriverjs to ensure we don't break 4.7.3 with 4.7.4.

Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, fine with leaving for a separate PR the ts tests + updating the rest of the packages to use the esbuild plugin we discussed

@straker straker merged commit b07d38c into develop Nov 21, 2023
30 checks passed
@straker straker deleted the webdriverjs-default-export branch November 21, 2023 20:17
@github-actions github-actions bot mentioned this pull request Nov 30, 2023
@dequejenn dequejenn mentioned this pull request Dec 5, 2023
@github-actions github-actions bot mentioned this pull request Jan 9, 2024
@github-actions github-actions bot mentioned this pull request Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix default commonJs export
3 participants