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

Switch to ESM for internals #2741

Merged
merged 7 commits into from
Jun 6, 2021
Merged

Switch to ESM for internals #2741

merged 7 commits into from
Jun 6, 2021

Conversation

novemberborn
Copy link
Member

@novemberborn novemberborn commented Apr 26, 2021

  • Make sure the entrypoints remain usable in a CJS environment
  • Convert all other internal files to ESM

@novemberborn
Copy link
Member Author

novemberborn commented May 2, 2021

Progress so far:

  • lib/ is ESM
    • I had to load yargs as CJS since it behaved differently
    • Line number parsing code is still CJS because it attempts to load some dependencies lazily and I didn't want to make it asynchronous
  • Shared worker filenames must now be file:// URLs, but you can provide a URL instance
  • Shared workers are always loaded through a dynamic import
  • test/ is ESM, except for the tests that would have required top-level await or too much refactoring
  • The XO config is now a JSON file
  • I'm enforcing import ordering and formatting beyond what XO does
  • I'm hoping code coverage isn't decreased
  • test-tap/ still needs migrating

@@ -1,3 +1,5 @@
import {URL} from 'url';
Copy link
Member Author

Choose a reason for hiding this comment

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

@sindresorhus is this idomatic?

Copy link
Member

Choose a reason for hiding this comment

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

No, but TS forces us to do it: DefinitelyTyped/DefinitelyTyped#34960

@novemberborn
Copy link
Member Author

@sindresorhus feedback welcome, though I'm not expecting you to review each file 😉 To be honest neither am I, as long as the tests pass I think we'll be OK.

const codeExcerpt = require('code-excerpt');
const truncate = require('cli-truncate');
const chalk = require('./chalk').get();
import fs from 'fs';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import fs from 'fs';
import fs from 'node:fs';

Any thoughts on switching to this while you're at it? I plan to enforce it in XO soon.

nodejs/node#38343

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be happy to let XO fix & enforce that when the time comes 😄

lib/provider-manager.js Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
import {expectType} from 'tsd';
import test, {ExecutionContext, Macro} from '..'; // eslint-disable-line import/no-unresolved

import test, {ExecutionContext, Macro} from '..';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import test, {ExecutionContext, Macro} from '..';
import test, {ExecutionContext, Macro} from '../index.js';

You should use full name and extension for the TS stuff too.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's is no index.js file, but there is an exports map. Which the linter rules don't seem to deal with, so I have direct imports of entrypoints/main.mjs elsewhere. But then TypeScript can't find the type definition. Hence .. here.

test/config/loader.js Outdated Show resolved Hide resolved
test/shared-workers/lifecycle/fixtures/teardown.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Member

I had to load yargs as CJS since it behaved differently

New version was just released. Maybe it's better: https://github.com/yargs/yargs/releases/tag/v17.0.0

@novemberborn
Copy link
Member Author

I had to load yargs as CJS since it behaved differently

New version was just released. Maybe it's better: yargs/yargs@v17.0.0 (release)

I'm always happy once I figured out how yargs works… not sure I want to deal with its breaking changes in this here PR.

@novemberborn
Copy link
Member Author

Test file paths within shared workers are now file URLs.

XO should pass, but the tap tests are not passing yet.

@novemberborn
Copy link
Member Author

Refactor and deploy some tricks so that ava/plugin is usable in a CJS environment, even if the rest of AVA's internals become ESM.
* Load yargs as CJS due to strange compatibility issues
* Keep line number parsing code as CJS because it attempts to lazy-load dependencies
* Shared workers are always loaded through a dynamic import
* Shared worker filenames must now be file:// URLs, but you can provide a URL instance
* Test file paths within shared workers are now file:// URLs
* Error serialization and code excerpts now support ESM
* Title prefixes in the reporters now correctly skip all test file extensions, not just `js`
@novemberborn
Copy link
Member Author

Tests pass locally… 😉

@sindresorhus
Copy link
Member

I haven't actually tested this, but the code changes look good to me.

@iambumblehead
Copy link

@novemberborn I'm looking forward to using this in AVA's next update :)

@novemberborn
Copy link
Member Author

I believe the test failures are due to either a Node.js bug or are intermittent. I'm going to merge this.

@novemberborn novemberborn marked this pull request as ready for review June 6, 2021 13:38
@novemberborn novemberborn dismissed sindresorhus’s stale review June 6, 2021 13:39

Addressed or deferred.

@novemberborn novemberborn merged commit c57067b into main Jun 6, 2021
@novemberborn novemberborn deleted the use-esm branch June 6, 2021 13:39
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.

Magic assert doesn't seem to work with "type": "module"
3 participants