-
Notifications
You must be signed in to change notification settings - Fork 83
chore(typescript): change moduleResolution #1605
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
base: master
Are you sure you want to change the base?
Changes from all commits
4ce46f3
1cc4d4d
08c1467
13fcb50
ec996e5
c210da6
1aa1232
bf4b00f
0882d51
ddd2837
4a7e963
ca74d7c
2b9a2e9
9086e84
5e58c52
5511457
ec033a9
944fd75
3dc97e4
2957d4d
2f55bf1
db0eb65
17f77bd
008fefe
6e97cbb
b43d2ca
e491985
7960f48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated Typescript to support loading ES modules in CommonJS synchronously (without This is needed to support the latest version of |
||
}, | ||
"resolutions": { | ||
"elliptic": "^6.6.1", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needed when exporting types with |
||
export { MockDataAccess } from './mock-data-access'; | ||
export { NoPersistDataWrite } from './no-persist-data-write'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import * as AJV from 'ajv'; | ||
import { Ajv } from 'ajv'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Later, I tested again with Ajv v6, this time with |
||
import addFormats from 'ajv-formats'; | ||
import * as jsonSchema from 'ajv/lib/refs/json-schema-draft-06.json'; | ||
import * as schemaAddress from './format/address.json'; | ||
import { formats } from './format'; | ||
|
@@ -9,7 +10,8 @@ import { formats } from './format'; | |
* @return object.valid == true if the json is valid, object.valid == false and object.errors otherwise. | ||
*/ | ||
export function validate(data: any): any { | ||
const validationTool = new AJV().addMetaSchema(jsonSchema).addSchema(schemaAddress); | ||
const validationTool = new Ajv().addMetaSchema(jsonSchema).addSchema(schemaAddress); | ||
addFormats(validationTool); | ||
|
||
// Check the meta information | ||
if (!data.meta) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,14 +50,14 @@ | |
"graphql-request": "6.1.0", | ||
"graphql-tag": "2.12.6", | ||
"satoshi-bitcoin": "1.0.4", | ||
"tslib": "2.5.0" | ||
"tslib": "2.8.1" | ||
}, | ||
"devDependencies": { | ||
"@babel/helper-get-function-arity": "7.16.7", | ||
"@graphql-codegen/cli": "4.0.1", | ||
"@graphql-codegen/typescript": "4.0.1", | ||
"@graphql-codegen/typescript-document-nodes": "4.0.1", | ||
"@graphql-codegen/typescript-graphql-request": "6.0.1", | ||
"@graphql-codegen/typescript-graphql-request": "6.2.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixes the generated GraphQL code that previously contained |
||
"@graphql-codegen/typescript-operations": "4.0.1", | ||
"@graphql-codegen/typescript-resolvers": "4.0.1", | ||
"@jridgewell/gen-mapping": "0.3.2", | ||
|
@@ -66,8 +66,8 @@ | |
"jest": "29.5.0", | ||
"jest-junit": "16.0.0", | ||
"source-map-support": "0.5.19", | ||
"ts-jest": "29.1.0", | ||
"ts-jest": "29.3.2", | ||
"ts-node": "10.9.1", | ||
"typescript": "5.1.3" | ||
"typescript": "5.8.3" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,8 @@ | |
import { CurrencyTypes } from '@requestnetwork/types'; | ||
import { NearChains } from '@requestnetwork/currency'; | ||
import { GraphQLClient } from 'graphql-request'; | ||
import { Block_Height, Maybe, getSdk } from './generated/graphql'; | ||
import { Block_Height, getSdk, Maybe } from './generated/graphql'; | ||
import { getSdk as getNearSdk } from './generated/graphql-near'; | ||
import { RequestConfig } from 'graphql-request/src/types'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This import is not compatible with ESM. We can't do such type of imports anymore, as |
||
|
||
const THE_GRAPH_STUDIO_URL = | ||
'https://api.studio.thegraph.com/query/67444/request-payments-$NETWORK/version/latest'; | ||
|
@@ -41,6 +40,8 @@ export type TheGraphQueryOptions = { | |
blockFilter?: Maybe<Block_Height>; | ||
}; | ||
|
||
type RequestConfig = (typeof GraphQLClient.prototype)['requestConfig']; | ||
|
||
export type TheGraphClientOptions = RequestConfig & { | ||
/** constraint to select indexers that have at least parsed this block */ | ||
minIndexedBlock?: number | undefined; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
forts-jest
to work withmodule=nodenext
(see explanation in thetsconfig.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, becausets-jest
was back to consuming a normal amount of memory. However this change seems to improve test time quite a bit:master
branch: 2m05sisolatedModules=true
: 2m05sisolatedModules=true
+JEST_MAX_WORKER=50%
: 1m21sIn 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 newJEST_MAX_WORKERS
environment variable I introduced could be extended later on to other jobs if needed, to reduce potential flakiness or increase CI speed.