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

[api] implement verbatim module syntax in tsconfig #3528

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

freemvmt
Copy link
Contributor

@freemvmt freemvmt commented Aug 16, 2024

This PR is currently was serving a purpose (as a rebase platform) in troubleshooting errors with Jest in #3453 (I explain my findings below), but it may also be worth merging at some point on its own merit.

The majority of the actual import ... fixes were done automatically using the consistent-type-imports eslint rule. I note that the typescript-eslint folks don't recommend using verbatimModuleSyntax and this rule together, but I'm not sure I'm convinced. I think any conflict here is outweighed by the value of being able to autofix lints? But open to being wrong!

If we'd rather bin the new eslint rule, it has largely served its purpose in automating the initial round of fixes, and TypeScript should flag any new failure to adhere to verbatimModuleSyntax in-IDE anyway, so it's not a significant loss.

Exploration

See the No transformations between module systems section here for an important corollary of imposing verbatimModuleSyntax (which was introduced in TS v5 and implies isolatedModules, importsNotUsedAsValues and preserveValueImports, the latter two of which are thereafter deprecated):

ESM syntax cannot be used in files that will emit CommonJS syntax.

More than any other switch I can throw, this change seems to cause TypeScript to aggressively enforce that every module is ESM-compliant, or else it will throw an error.

This is exactly why I have to add the ts-node options to tsconfig.json, and adapt the tsconfig.test.json which the jest.config.ts reads. As per this comment, both tools will error out if they abide by the "verbatimModuleSyntax": true option in the root tsconfig.

That is, if we comment out "verbatimModuleSyntax": false in the ts-node specific compiler options in tsconfig.json, we get the following instead:

Error: Jest: Failed to parse the TypeScript config file /home/dan/code/planx-new/api.planx.uk/jest.config.ts
  TSError: ⨯ Unable to compile TypeScript:
jest.config.ts:49:1 - error TS1286: ESM syntax is not allowed in a CommonJS module when 'verbatimModuleSyntax' is enabled.

49 export default config;

The thing to understand here is that Jest uses ts-node to transpile the jest.config.ts file into JS in order to parse it, and it instructs ts-node to consider all files as commonJS, in a way which is difficult to overwrite from tsconfig.

This is not too much of a problem - ts-node is only used for this specific purpose, and we can probably stand one file being interpreted as cjs for the sole purpose of initiating tests. After the config is read in, Jest takes over, and the first thing is does it try to transform all files into JS using ts-jest as the transpiler.

If however, we comment out tsconfig: ... in jest.config.ts, we get something like the following:

 FAIL  ./server.test.js
  ● Test suite failed to run

    server.ts:3:8 - error TS1286: ESM syntax is not allowed in a CommonJS module when 'verbatimModuleSyntax' is enabled.

Now this is a helpful error, and illustrates the real problem at hand. It tells us that ts-jest is genuinely not interpreting server.ts as an ESM module, despite the "type": "module" in package.json, the useEsm and extensionsToTreatAsEsm options in the jest config, etc!

Conclusion

This demonstrates that perhaps the Jest errors are more genuine/instructive than I imagined. Despite all the config in the world, jest/ts-jest refuse to recognise our api.planx.uk codebase as being composed of ESM modules. Perhaps the solution is simply to not enforce ESM for tests? 🤔

Copy link

github-actions bot commented Aug 16, 2024

Removed vultr server and associated DNS entries

@freemvmt freemvmt changed the title [api] implement verbatim module syntax [api] implement verbatim module syntax in tsconfig Aug 16, 2024
@freemvmt freemvmt marked this pull request as ready for review August 16, 2024 21:00
@freemvmt freemvmt requested a review from a team August 16, 2024 21:40
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Just catching up here @freemvmt, thanks so much for the detailed write up - much appreciated.

We're currently facing a few Jest issues in the frontend also and are considering the switch to Vitest. I don't want to muddy the waters too much or add another big config heavy task to your plate, but it might be worth a timeboxed investigation to see if a switch could solve a few of your issues without too much work?

@freemvmt
Copy link
Contributor Author

freemvmt commented Sep 6, 2024

As per #3453, we resolved the issue with Jest which this PR was intended to help investigate by migrating our entire test suite for api.planx.uk to Vitest, which had immediate results!

Implementing the verbatimModuleSyntax module may still be a desirable change, to make our TypeScript code as explicit as possible and safeguard against any issues with per-file transpilation, but is no longer a dependency for the Microsoft single sign on work, so this is a decision we can take in the fullness of time.

Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

I know that this was originally introduced to resolve jest pain points, but now we've resolved that with vitest we need to decide what to do here as it's no longer a strict pre-requisite.

The authority I trust on all things TypeScript recommends using verbatimModuleSyntax so I'm all in favour of this change - it's more explicit and clear so let's go ahead and make this change. Especially as you've done all the groundwork here!

Once the PR is rebased I'll be happy to approve and merge this one.

move ts-node to devDependencies

add consistent-type-imports eslint rule as linting proxy for verbatimModuleSyntax rule
@freemvmt freemvmt force-pushed the implement-verbatim-module-syntax branch from 65119b4 to 70c26be Compare September 12, 2024 14:11
@freemvmt freemvmt merged commit e731ef6 into main Sep 12, 2024
12 checks passed
@freemvmt freemvmt deleted the implement-verbatim-module-syntax branch September 12, 2024 15:33
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.

2 participants