chore(typescript): change moduleResolution #1605
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update applies a broad set of dependency version bumps across multiple package.json files throughout the repository, notably upgrading TypeScript, tslib, ts-jest, and several other libraries. The TypeScript compiler configuration (tsconfig.json) is updated to use the "nodenext" module system and enables "esModuleInterop" and "isolatedModules". The CircleCI configuration is adjusted to set a JEST_MAX_WORKERS environment variable for concurrent Jest test runs. Jest's configuration is made responsive to this environment variable. Several internal type imports are refactored to avoid reliance on internal or legacy paths by deriving types from GraphQLClient prototypes. The Ajv JSON schema validator integration is updated to use ajv-formats, affecting both code and test expectations. Additionally, multiple export statements are changed to type-only exports for clarity and correctness. Changes
Sequence Diagram(s)sequenceDiagram
participant CircleCI
participant Jest
participant Lerna
CircleCI->>Jest: Set JEST_MAX_WORKERS='50%'
Lerna->>Jest: Run 2 Jest commands concurrently
Jest->>Jest: Use maxWorkers from env if set
sequenceDiagram
participant Validator
participant Ajv
participant ajv-formats
Validator->>Ajv: Create Ajv instance
Validator->>ajv-formats: addFormats(Ajv)
Validator->>Ajv: Add meta schema, address schema
Validator->>Ajv: Validate data
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -52,7 +52,7 @@ | |||
| "npm-package-json-lint": "5.1.0", | |||
| "prettier": "2.8.8", | |||
| "prettier-plugin-solidity": "1.0.0-beta.19", | |||
| "typescript": "5.1.3" | |||
| "typescript": "5.8.3" | |||
There was a problem hiding this comment.
Updated Typescript to support loading ES modules in CommonJS synchronously (without await import()):
https://nodejs.org/api/modules.html#loading-ecmascript-modules-using-require
This is needed to support the latest version of graphql-request used in the payment-detection package.
| "@graphql-codegen/typescript-graphql-request": "6.0.1", | ||
| "@graphql-codegen/typescript-graphql-request": "6.2.0", |
There was a problem hiding this comment.
Fixes the generated GraphQL code that previously contained imports not compatible with latest module resolution.
| import { getSdk as getNearSdk } from './generated/graphql-near'; | ||
| import { RequestConfig } from 'graphql-request/src/types'; |
There was a problem hiding this comment.
This import is not compatible with ESM. We can't do such type of imports anymore, as graphql-request/src/types is not an exported module.
alexandre-abrioux
force-pushed
the
moduleresolution
branch
from
56aea10 to
c210da6
Compare
| "dotenv": "8.2.0", | ||
| "dotenv": "16.5.0", |
There was a problem hiding this comment.
There was an issue with how dotenv exported their types in older versions, resulting in error
TS7016: Could not find a declaration file for module dotenv.
|
@alexandre-abrioux: |
|
Thanks a lot @nkoreli 🙏 |
| import * as AJV from 'ajv'; | ||
| import { Ajv } from 'ajv'; |
There was a problem hiding this comment.
With the old version (v6) of Avj there was a TypeScript issue, but only during tests. I fixed it by updating Avj to v8 and handling the breaking changes. This was before I found out that ts-jest needs isolatedModules=true to support module=nodenext.
Later, I tested again with Ajv v6, this time with isolatedModules=true, and it also works. But I figured I would keep this change since I went through the trouble of updating it.
| "module": "commonjs", | ||
| "module": "nodenext", | ||
| "moduleResolution": "nodenext", | ||
| "esModuleInterop": true, |
There was a problem hiding this comment.
esModuleInterop=true is now the default with module=nodenext: https://www.typescriptlang.org/tsconfig/#esModuleInterop ; but we still specify it so that other tools (IDE, eslint, ts-jest) are all aligned with it.
For instance, there was an issue with ts-jest showing warnings when this field was not set, which was also fixed by updating the ts-jest package later on: kulshekhar/ts-jest#4266 ; but I kept esModuleInterop=true for clarity.
| environment: | ||
| # Lerna starts 2 Jest commands at the same time (see above --concurrency=2), | ||
| # so we use 50% of our CPU cores on each | ||
| JEST_MAX_WORKERS: '50%' |
There was a problem hiding this comment.
This change is not mandatory.
While fiddling with our TS config, I had some issues with the CI, especially before I found out that we had to activate isolatedModules=true for ts-jest to work with module=nodenext (see explanation in the tsconfig.json file). At this point, ts-jest was consuming a considerable amount of RAM during tests. The job highlighted above (test-unit) constantly consumed 100% of the RAM and failed with timeouts or out-of-memory errors.
While digging, I found that configuring Jest to only use 4 workers (50% of CPU cores) instead of 8 (default 100% CPU cores) completely stabilized the tests, even with this buggy ts-jest config consuming a lot of memory.
After enabling isolatedModules=true, I could have reverted that change, because ts-jest was back to consuming a normal amount of memory. However this change seems to improve test time quite a bit:
masterbranch: 2m05s- PR branch with
isolatedModules=true: 2m05s - PR branch with
isolatedModules=true+JEST_MAX_WORKER=50%: 1m21s
In the end, even if this change is not mandatory, I have kept it as it made our pipeline faster.
Side note that confirms this: Adobe dev team advises in their blog using a maximum of 50% CPU cores for Jest workers, especially on small machines like CI runners, so as to leave some room for Jest to spawn NodeJS processes.
Here, I have only modified the configuration for this specific test-unit job, which was causing me issues, but the new JEST_MAX_WORKERS environment variable I introduced could be extended later on to other jobs if needed, to reduce potential flakiness or increase CI speed.
| "module": "nodenext", | ||
| "moduleResolution": "nodenext", | ||
| "esModuleInterop": true, | ||
| "isolatedModules": true, |
There was a problem hiding this comment.
ts-jest requires isolatedModules=true with module=nodenext, see:
https://kulshekhar.github.io/ts-jest/docs/guides/esm-support/#using-hybrid-module-values
This does not change the emitted JS files; it only adds warnings when using TypeScript features that are not compatible with single-module transpilation.
| @@ -3,6 +3,6 @@ export { CombinedDataAccess } from './combined-data-access'; | |||
| export { DataAccessWrite } from './data-write'; | |||
| export { DataAccessRead } from './data-read'; | |||
| export { PendingStore } from './pending-store'; | |||
| export { DataAccessBaseOptions } from './types'; | |||
| export type { DataAccessBaseOptions } from './types'; | |||
There was a problem hiding this comment.
needed when exporting types with isolatedModules=true
github-actions
bot
requested review from
aimensahnoun,
kevindavee,
leoslr and
MantisClone
alexandre-abrioux
removed request for
aimensahnoun,
kevindavee,
leoslr and
MantisClone
alexandre-abrioux
force-pushed
the
moduleresolution
branch
from
ed71a79 to
7960f48
Compare
github-actions
bot
requested review from
kevindavee,
leoslr,
rodrigopavezi and
yomarion
alexandre-abrioux
requested review from
MantisClone,
aimensahnoun and
yomarion
and removed request for
kevindavee,
leoslr and
yomarion
In this PR, I'm proposing to change TypeScript's
moduleandmoduleResolutionto the latest recommended versions (see the section below for TypeScript recommendations). This would allow us to use ESM features in CommonJS, and forbid us to use non-ESM compatible imports, which should ease the transition to ESM later on. We would also gain:require(esm): allows to import ESM modules synchronously in CommonJS, without the need forawait import()exportsproperty, which allows importing part of a module withimport {} from module/part. e.g.dotenv/config. This change is required for chore(WIP): migrate to @noble/curves and eciesjs #1236TypeScript Documentation
module
https://www.typescriptlang.org/docs/handbook/modules/reference.html#commonjs
moduleResolution
https://www.typescriptlang.org/docs/handbook/modules/reference.html#node10-formerly-known-as-node
Issue to address before merge
@hinkal/commondoes not export types, thetypesfield is missing in thepackage.json:https://cdn.jsdelivr.net/npm/@hinkal/[email protected]/package.json
Fixed in
0.2.12, https://cdn.jsdelivr.net/npm/@hinkal/[email protected]/package.jsonSummary by CodeRabbit
New Features
Bug Fixes
Chores