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

Move to ESM, remove index.d.ts requirement #186

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

tommy-mitchell
Copy link
Contributor

@tommy-mitchell tommy-mitchell commented Mar 15, 2023

Closes #183.
Closes #191.
Closes #175.

Checklist:

  • Update tsconfig.tsd.json
    • Set module to node16
    • Set target to ES2020
  • Update package.json
    • Add "type": "module"
    • Update to AVA v5.2.0, add tsx for transpilation
    • Inline clean script
  • Add .js to relative imports in source and test files
  • Make sure that Node builtins use the node: protocol
  • Update dependencies
  • Update tsd's defaults (tsd doesn't catch missing extensions in declaration files #191)

We should also update the assertion definitions to use const type parameters.


Speaking of tests, trying to run AVA causes errors with @tsd/typescript:

npx ava source/test/test.ts

(node:69218) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
  Uncaught exception in source/test/test.ts

  SyntaxError: The requested module '@tsd/typescript' does not provide an export named 'TypeFormatFlags'

  ✘ source/test/test.ts exited with a non-zero exit code: 1
  ─

  1 uncaught exception

In addition, updating AVA means that t.throwsAsync can also return undefined:

test('failure', async t => {
	const cwd = path.join(__dirname, 'fixtures/failure');
	const {exitCode} = await t.throwsAsync<ExecaError>(execa('../../../cli.js', {cwd}));
	//=> Property 'exitCode' does not exist on type 'ExecaError | undefined'.

	t.is(exitCode, 1);
});

cc: @sindresorhus

@tommy-mitchell
Copy link
Contributor Author

Using tsx would resolve #175 as well.

@mrazauskas
Copy link
Contributor

By the way, this is 100% breaking change for programmatic API users. Perhaps #75 could be considered? Would be enough to return an object. This would make it easier to add any additional props in the future.

The #75 was needed for jest-runner-tsd, but it was rather unclear if that will ever happen. I fork tsd and released tsd-lite. You might be interested to look around:

@sindresorhus
Copy link
Collaborator

What's the benefit of using .tsx? It's not clear to me.

@sindresorhus
Copy link
Collaborator

@sindresorhus
Copy link
Collaborator

sindresorhus commented Mar 16, 2023

SyntaxError: The requested module '@tsd/typescript' does not provide an export named 'TypeFormatFlags'

The ES module resolution is stricter about exports. Make sure it's actually exported correctly.

@tommy-mitchell
Copy link
Contributor Author

What's the benefit of using .tsx? It's not clear to me.

tsx is a tool like ts-node for transpiling TS in-memory. The name comes from modeling npx. It’s faster than ts-node and works better with ESM, but skips type checking.

The naming is unfortunate.

avajs/ava#2593 (comment)

@sindresorhus
Copy link
Collaborator

tsx is a tool like ts-node for transpiling TS in-memory. The name comes from modeling npx. It’s faster than ts-node and works better with ESM, but skips type checking.

Ok. We definitely want type checking to ensure they are valid. But we could also just run tsc --noEmit before to type check.

@sindresorhus
Copy link
Collaborator

Let me know if anything is blocked on me commenting on something.

@tommy-mitchell
Copy link
Contributor Author

But we could also just run tsc --noEmit before to type check.

I decided to go with that as well. I've been doing more locally, you're fine.

SyntaxError: The requested module '@tsd/typescript' does not provide an export named 'TypeFormatFlags'

TypeFormatFlags is an enum exported bytypescript itself. I'm going to see if using ts-node works instead.


Re: #75, should we close that PR and open a new one? It's definitely stale, and the diff is pretty extraneous at this point. I can open a reminder issue.

@sindresorhus
Copy link
Collaborator

Re: #75, should we close that PR and open a new one? It's definitely stale, and the diff is pretty extraneous at this point. I can open a reminder issue.

Yes

@tommy-mitchell
Copy link
Contributor Author

Seems like the issue with the tests is that they are being transpiled to CJS, but since "type": "module" is set the tests throw.

Related, is there a reason that we have tsconfig.tsd.json and not tsconfig.json?

@sindresorhus
Copy link
Collaborator

Related, is there a reason that we have tsconfig.tsd.json and not tsconfig.json?

#100

@tommy-mitchell
Copy link
Contributor Author

I've uploaded some more changes (tests are still failing). I'll resolve the merge conflicts.

A couple highlights: moved to xo for linting, updated the test script, and consolidated the tests via macros (still need to do so for the CLI tests).

@tommy-mitchell
Copy link
Contributor Author

SyntaxError: The requested module '@tsd/typescript' does not provide an export named 'TypeFormatFlags'

The ES module resolution is stricter about exports. Make sure it's actually exported correctly.

Turns out this is because the TypeScript compiler is CJS: microsoft/TypeScript#32949

@tommy-mitchell tommy-mitchell mentioned this pull request Aug 16, 2023
@sindresorhus
Copy link
Collaborator

@tommy-mitchell Just a reminder that this is still open. If you're simply busy, feel free to ignore this.

@tommy-mitchell
Copy link
Contributor Author

@sindresorhus this is on my todo list, I’ve got some local changes that are unpushed. If you have some time, I’d love your feedback on #196

);

const combinedCompilerOptions = {
...tsConfigCompilerOptions,
...packageJsonCompilerOptions,
};

const module = combinedCompilerOptions.module ?? ModuleKind.CommonJS;
const module = combinedCompilerOptions.module ?? ts.ModuleKind.CommonJS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const module = combinedCompilerOptions.module ?? ts.ModuleKind.CommonJS;
const module = combinedCompilerOptions.module ?? (pkg.type === 'module' ? ts.ModuleKind.Node16 : ts.ModuleKind.CommonJS);

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on these defaults?

function getModuleResolution(module: ts.ModuleKind): ts.ModuleResolutionKind {
	if (module === ts.ModuleKind.NodeNext) {
		return ts.ModuleResolutionKind.NodeNext;
	}

	if (module === ts.ModuleKind.Node16) {
		return ts.ModuleResolutionKind.Node16;
	}

	if (module === ts.ModuleKind.ES2020 || module === ts.ModuleKind.ES2022 || module === ts.ModuleKind.ESNext) {
		return ts.ModuleResolutionKind.Bundler;
	}

	// Fallback to Node16
	return ts.ModuleResolutionKind.Node16;
}

@tommy-mitchell tommy-mitchell changed the title Move to ESM Move to ESM, remove index.d.ts requirement Feb 23, 2024
@tommy-mitchell
Copy link
Contributor Author

tommy-mitchell commented Feb 23, 2024

Ended up removing the index.d.ts requirement in this PR because it was giving me trouble with tests. Merged the changes from main. Next steps:

  • Update tsd's defaults
  • Update PR description (e.g. this also includes a change the build script, added @sindresorhus/tsconfig, support for more ts extensions)
  • Fix test failures (CLI tests in particular don't seem to be working)
  • Update documentation to reflect ESM / index.d.ts changes (more doc improvements can be saved for another PR)

@tommy-mitchell
Copy link
Contributor Author

tommy-mitchell commented Feb 23, 2024

Are we still supporting testing types in CJS packages after moving to ESM?

@tommy-mitchell
Copy link
Contributor Author

tommy-mitchell commented Feb 23, 2024

Seems like xo is failing because it can't find a tsconfig.json, even though I set it in parserOptions.project:

file:///home/runner/work/tsd/tsd/node_modules/xo/lib/options-manager.js:163
		options.tsConfig = tsConfigResolvePaths(getTsconfig(options.tsConfigPath).config, options.tsConfigPath);
		                                                                         ^

TypeError: Cannot read properties of null (reading 'config')
    at handleTSConfig (file:///home/runner/work/tsd/tsd/node_modules/xo/lib/options-manager.js:163:76)
    at mergeWithFileConfig (file:///home/runner/work/tsd/tsd/node_modules/xo/lib/options-manager.js:137:19)
    at async parseOptions (file:///home/runner/work/tsd/tsd/node_modules/xo/lib/options-manager.js:578:51)
    at async Promise.all (index 0)
    at async getOptionGroups (file:///home/runner/work/tsd/tsd/node_modules/xo/lib/options-manager.js:594:21)
    at async Object.lintFiles (file:///home/runner/work/tsd/tsd/node_modules/xo/index.js:80:17)
    at async file:///home/runner/work/tsd/tsd/node_modules/xo/cli.js:212:18

@tommy-mitchell
Copy link
Contributor Author

Seems like there should be a way for us to just tsconfig.json directly, I'll try to see if I can do that without running into the issues from #100.

@sindresorhus
Copy link
Collaborator

Are we still supporting testing types in CJS packages after moving to ESM?

No

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.

tsd doesn't catch missing extensions in declaration files Move to ESM Ability to run tests in watch mode
3 participants