feat: upgrade packages to use node 22 #1595
Conversation
WalkthroughThis pull request primarily upgrades Node.js version requirements across the repository. The changes update various CI configurations and package engine declarations—from Node 18.x to Node 22.x—in files such as CircleCI configuration, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as SubgraphClient
participant G as GraphQLClient
C->>S: Call getBlockNumber or fetchAndFormat()
S->>S: ensureGraphQLClientInitialized()
alt GraphQL client not initialized
S->>S: initGraphQLClient()
S->>G: Dynamically import and instantiate
G-->>S: Return initialized client
end
S->>G: Execute GraphQL query
G-->>S: Return response
S-->>C: Return processed data
Assessment against linked issues
Possibly related PRs
Suggested reviewers
🪧 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 (
|
aimensahnoun
force-pushed
the
1492-support-node-v20-before-end-of-life-on-april-30-2025
branch
from
c89e272 to
5ca36ab
Compare
…structor options to omit headers
…graphql RequestConfig
…es for Jest configuration
…t files and update transformIgnorePatterns
…de ts-jest transformations and adjust transformIgnorePatterns
… refine transformIgnorePatterns to include @superfluid-finance/sdk-core
…ings to support ES modules and TypeScript
…n with new package versions and refine module handling
…unnecessary console logs from HttpDataAccessConfig
…ge configurations
aimensahnoun
changed the title
aimensahnoun
force-pushed
the
1492-support-node-v20-before-end-of-life-on-april-30-2025
branch
from
90b1bb1 to
bcf0665
Compare
…ript support in request-node package
github-actions
bot
requested review from
kevindavee,
rodrigopavezi and
yomarion
aimensahnoun
requested review from
MantisClone and
leoslr
and removed request for
kevindavee and
yomarion
alexandre-abrioux
left a comment
alexandre-abrioux
left a comment
There was a problem hiding this comment.
If this setup with Babel is required to run the tests, does that mean that our library consumers will also have to implement such a config?
As an alternative, have you tried importing asyncronously the graphql-request package?
const graphqlRequest = await import("graphql-request");; this can usually solve ESM module imports in a CommonJS codebase.
However, this solution can also conflict with TypeScript which may transpile await import() to require(). At Request Finance we had luck in the past using the following package to import ESM modules: https://github.com/Borewit/load-esm. It bypasses TypeScript so that asynchronous import() are not converted to require().
aimensahnoun
force-pushed
the
1492-support-node-v20-before-end-of-life-on-april-30-2025
branch
from
d43bf8f to
713b757
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/thegraph-data-access/src/subgraph-client.ts (3)
21-24: Lazy initialization is fine, but be aware of concurrency.
Usingprivate graphql!: GraphQLClient;with a boolean flag is straightforward. However, if multiple threads or async calls race to initialize, you could get duplicate in-flight calls toinitGraphQLClient. Consider adding synchronization if concurrency is expected.
35-35: Re-check concurrency ininitialize().
CallingensureGraphQLClientInitializedis sensible for lazy loading. Ifinitialize()is called multiple times concurrently, you might get repeated calls. If concurrency is not a concern, this is fine.
135-140: Check for race conditions inensureGraphQLClientInitialized().
If multiple calls race in parallel beforegraphQLClientInitializedtoggles totrue, you might create multiple client instances. If concurrency is possible, consider adding a lock or WaitGroup-like mechanism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/integration-test/package.json(1 hunks)packages/request-node/package.json(1 hunks)packages/thegraph-data-access/src/subgraph-client.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/request-node/package.json
- packages/integration-test/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (8)
packages/thegraph-data-access/src/subgraph-client.ts (8)
2-2: Importing 'Variables' from 'graphql-request' is valid.
No concerns—this is the correct type to represent query variables used infetchAndFormatand other methods.
18-18: Ensure the customheaderstype won't conflict with advanced usage.
Omitting'headers'fromRequestConfigand reintroducingheaders?: Record<string, string>might exclude certain header transformations (e.g. multi-value or function-based headers). Verify that no usage needs other header forms.
40-40: Correct usage of lazy initialization ingetBlockNumber().
This is consistent with the pattern of deferring GraphQL client creation until needed, preventing unnecessary overhead in some scenarios.
64-64: Ensuring the client is ready ingetTransactionsByTopics().
This follows the same lazy initialization pattern. Good job keeping it consistent.
94-94: Verifying the GraphQL client infetchAndFormat().
Again, callingensureGraphQLClientInitialized()just before the request is made ensures correctness if the client was never set up beforehand.
129-133: Dynamic import ininitGraphQLClientis a flexible approach for lazy loading.
Delaying the import ofgraphql-requestcan reduce startup overhead if the client isn’t often used. Implementation looks solid.
13-13:❓ Verification inconclusive
Consider verifying the import path for
RequestConfig.
RequestConfigmight be re-exported at a top-level fromgraphql-request, which would be simpler than referencing a legacy path.Below is a script to check if
RequestConfigcan be safely imported from'graphql-request':
🏁 Script executed:
#!/bin/bash # Searches if `RequestConfig` is re-exported by the top-level of `graphql-request`. rg -A 5 "export .*RequestConfig" node_modules/graphql-requestLength of output: 132
Action: Manually Verify the Import Path for
RequestConfigThe automated check couldn’t confirm a re-export at the top-level because the search failed due to the absence of the
node_modules/graphql-requestdirectory in this environment. Please manually verify whether your version ofgraphql-requestre-exportsRequestConfig. If it does, update the import inpackages/thegraph-data-access/src/subgraph-client.tsto:import type { RequestConfig } from 'graphql-request';Otherwise, continue using the current legacy path:
import type { RequestConfig } from 'graphql-request/build/legacy/helpers/types';
29-31:❓ Verification inconclusive
Confirm backward compatibility with new constructor signature.
Switching theoptionsparameter fromRequestConfigtoClientConfigmay break external usage if they previously relied on the originalRequestConfigshape. Confirm no consumers are impacted.Here’s a script to locate references to
new SubgraphClient:
🏁 Script executed:
#!/bin/bash # Searches for all constructor calls to SubgraphClient to verify argument shape ast-grep --pattern $'new SubgraphClient($_, $_)'Length of output: 158
Ensure Updated Constructor Signature Maintains Backward Compatibility
Our verification found a single internal usage atpackages/thegraph-data-access/src/data-access.ts(line 18) whereSubgraphClientis instantiated as follows:const graphqlClient = new SubgraphClient(url, rest);Please confirm that the
restargument in this call conforms to the newClientConfigrequirements. Additionally, if this package is published for external consumption, double-check that no external consumers are relying on the olderRequestConfigshape.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/thegraph-data-access/src/queries.ts (1)
41-41: Consider thread-safety for lazy import.Storing
gqlTagin a module-level variable and lazily initializing it could, in rare concurrent scenarios, cause inconsistent states if multiple calls togetGqlTag()happen at once. Althoughimport()calls are typically cached, it might still be prudent to either guard initialization with a lock or simply perform top-level imports for a more predictable approach.Also applies to: 43-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/thegraph-data-access/src/queries.ts(1 hunks)packages/thegraph-data-access/src/subgraph-client.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/thegraph-data-access/src/subgraph-client.ts
🔇 Additional comments (3)
packages/thegraph-data-access/src/queries.ts (3)
33-39: Type definition looks good.The
Queriestype is clear and cohesive. It neatly encapsulates all relevant query strings for consistent usage throughout the codebase.
51-70: Good approach for organizing GraphQL fragments.Bundling the
TransactionsBodyFragmentwith lazygqlloading helps keep queries responsive and modular. Ensure that your build and bundling pipeline properly handles dynamic imports so that performance isn’t adversely affected.
72-125:❓ Verification inconclusive
Verify naming consistency and schema alignment with dataHash.
- The export name
GetTransactionByDataHashQuerydiffers slightly from the internal GraphQL query nameGetTransactionsByDataHash. While functional, consider aligning them for clarity.- The query references a field
dataHash, but theTransactiontype does not have adataHashproperty. Confirm that the subgraph schema includes this field and update theTransactiontype accordingly to avoid confusion.To confirm correctness, run the following script to search for references to
dataHashin the codebase and compare against any subgraph schema definitions:
🏁 Script executed:
#!/bin/bash # Validate references to 'dataHash' and ensure the subgraph schema includes it # Search for usage of 'dataHash' across the project rg -A 5 "dataHash" # If there's a subgraph schema, check for 'dataHash' field declarations fd "schema.graphql" | xargs cat | rg "dataHash" -A 5Length of output: 5001
Action Required: Address Naming Consistency and Confirm Schema Alignment for
dataHash
- Naming Consistency: The exported query is named
GetTransactionByDataHashQuerywhile the internal GraphQL query is declared asGetTransactionsByDataHash. For clarity, please align these naming conventions—either update the export to match the internal query or vice versa.- Schema Alignment: The query filters on a
dataHashfield; however, based on the verification script output, there’s no clear evidence in the subgraph schema (e.g., inschema.graphql) that theTransactiontype includes adataHashproperty. Please manually verify that the subgraph schema definesdataHashon theTransactiontype. If it doesn’t, update the schema accordingly or reconsider the query’s filter.
aimensahnoun
force-pushed
the
1492-support-node-v20-before-end-of-life-on-april-30-2025
branch
from
bcbe9d9 to
163bbec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/thegraph-data-access/src/queries.ts(1 hunks)packages/thegraph-data-access/src/subgraph-client.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/thegraph-data-access/src/subgraph-client.ts
🔇 Additional comments (2)
packages/thegraph-data-access/src/queries.ts (2)
33-39: Good addition of the Queries typeThe new Queries type provides clear structure for the GraphQL queries being exported, improving type safety and making the API contract more explicit.
41-49: Well implemented lazy loading pattern for the gql tagThe lazy loading approach for the GraphQL tag is a good pattern for handling ES modules from
graphql-request. This approach properly caches the importedgqltag and handles the asynchronous nature of dynamic imports, ensuring the import only happens once.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/thegraph-data-access/src/queries.ts (2)
45-51: Consider adding error handling for dynamic import.
loadEsm('graphql-request')might fail under certain conditions (e.g., unavailable network resources, corrupted packages, etc.). Adding a try-catch or fallback can safeguard against runtime errors.
53-128: Potential performance improvement: caching the queries object.
getQueries()returns a new object each time, which can be suboptimal if called frequently. Consider caching the result to avoid redundant object creation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
packages/thegraph-data-access/package.json(2 hunks)packages/thegraph-data-access/src/queries.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (4)
packages/thegraph-data-access/package.json (2)
22-22: No concerns with Node.js version upgrade.Upgrading to Node.js >=22.0.0 aligns well with the PR objective to standardize on Node.js 22.
48-49:✅ Verification successful
Confirm package versions.
The
graphql-requestdependency has been raised to 7.1.2 andload-esmhas been introduced. Although these updates fit the PR’s Node 22 objective, it might be beneficial to confirm that 7.1.2 is indeed the most robust and stable option for your needs, and thatload-esmis compatible with your overall build and testing pipeline.
🏁 Script executed:
#!/bin/bash # Check for the latest version of graphql-request npm info graphql-request version # Check for available metadata on load-esm npm info load-esmLength of output: 872
Confirmed Dependency Versions
The updated dependencies in
packages/thegraph-data-access/package.jsonhave been verified. Specifically:
- graphql-request: Set to version 7.1.2, which is confirmed as the latest stable release.
- load-esm: Introduced at version ^1.0.2, and the latest version from npm is 1.0.2.
These changes align with the PR's Node 22 objective. No further changes are required, though please ensure that these updates integrate well with your build and testing pipeline.
packages/thegraph-data-access/src/queries.ts (2)
1-3: Good approach to dynamic ESM loading.Utilizing
load-esmfor on-demand importing ofgqlis a neat strategy that can reduce overhead in environments wheregqlis not always required.
37-43: Well-structured type definition.Exporting a dedicated
Queriestype consolidates and standardizes GraphQL queries into a single definition, improving type safety and maintainability.
aimensahnoun
force-pushed
the
1492-support-node-v20-before-end-of-life-on-april-30-2025
branch
from
4d571f8 to
548f460
Compare
|
@alexandre-abrioux that's a very good question. So I installed and ran the {
"name": "test-request-node",
"version": "1.0.0",
"type": "commonjs",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"keywords": [],
"author": "",
"license": "ISC",
"description": "",
"dependencies": {
"@requestnetwork/request-node": "file:requestnetwork-request-node-0.44.0.tgz"
}
}require("dotenv").config();
const { startNode } = require("@requestnetwork/request-node/dist/server");
// Start the request node
startNode().catch((error) => {
console.error("Failed to start request node:", error);
process.exit(1);
});And was able to read transactions:
|
alexandre-abrioux
left a comment
alexandre-abrioux
left a comment
There was a problem hiding this comment.
Nicely done! Thanks for testing 🙏
…ndency resolution; remove TypeScript preset from Babel configurations in integration-test and request-node packages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/integration-test/package.json (1)
44-48: Introduce Babel Core and Plugin DependenciesThe addition of
@babel/core,@babel/plugin-transform-modules-commonjs,@babel/plugin-transform-runtime, and@babel/preset-envin the devDependencies facilitates proper ES module transformation (especially for Jest). Make sure your Babel configuration file (e.g.,.babelrcorbabel.config.js) is updated accordingly to leverage these plugins.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/integration-test/babel.config.js(1 hunks)packages/integration-test/package.json(4 hunks)packages/request-node/babel.config.js(1 hunks)packages/request-node/package.json(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/request-node/babel.config.js
- packages/integration-test/babel.config.js
🔇 Additional comments (7)
packages/integration-test/package.json (3)
20-20: Update Node Engine Version RequirementThe engines section now specifies
"node": ">=22.0.0", which aligns with the PR objective to support Node.js 22. Please ensure that this change is also reflected in your CI and local development configurations.
68-68: Addition of babel-jest DependencyIntroducing
babel-jestensures that Jest can correctly transform ES modules during testing. This is essential given the ES module dependencies in the project.
79-80: Add @babel/runtime as a DependencyIncluding
"@babel/runtime": "7.23.9"provides runtime helpers that optimize Babel-transpiled code. Confirm that this version is consistent across packages.packages/request-node/package.json (4)
22-22: Upgrade Node Engine RequirementThe engines field now declares
"node": ">=22.0.0", ensuring that this package runs on Node.js 22. This update is consistent with the overall upgrade objective.
44-44: Include @babel/runtime in DependenciesAdding
"@babel/runtime": "7.23.9"in dependencies is a good move to leverage shared Babel helpers at runtime. Verify that this version matches the one used in other packages.
73-76: Update Babel Development DependenciesThe devDependencies now include essential Babel packages—
@babel/core,@babel/plugin-transform-modules-commonjs,@babel/plugin-transform-runtime, and@babel/preset-env—which will aid in transforming ES modules. Ensure that your Babel configurations are aligned with these updates.
84-84: Ensure Jest Transforms ES Modules with babel-jestThe addition of
babel-jestin the devDependencies ensures that Jest properly processes ES modules for testing. This aligns with the changes for Node 22 compatibility.
… and refine module handling in integration-test and request-node packages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/integration-test/package.json (1)
43-43: Empty Dependencies Object Notice:
An empty"dependencies": {}object has been added. If this is intentional (perhaps as a placeholder or for future packages), it is acceptable. Otherwise, consider removing it to keep the file clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
packages/integration-test/package.json(3 hunks)packages/request-node/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/request-node/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
packages/integration-test/package.json (3)
19-21: Node Engine Requirement Update:
The"engines"field now requires"node": ">=22.0.0", which aligns with the PR objective of upgrading to Node.js 22. Ensure that all environments (local development, CI/CD, and production) are configured accordingly to avoid any runtime discrepancies.
45-49: Addition of Babel Dependencies:
New Babel packages have been added—namely,@babel/core,@babel/plugin-transform-modules-commonjs,@babel/plugin-transform-runtime,@babel/preset-env, and@babel/runtime—all with pinned versions. These dependencies are essential for transforming ES modules into CommonJS, which is particularly important given the recent changes in thegraphql-requestpackage. Please ensure that your Babel configuration (e.g., in your Babel config file) utilizes these plugins properly.
70-70: Babel-Jest Integration for ESM Transformation:
The inclusion ofbabel-jest(version"29.7.0") facilitates proper transformation of ES modules in tests. This change is critical for resolving issues that arose with modules likegraphql-requestwhen run under Jest.
aimensahnoun
deleted the
1492-support-node-v20-before-end-of-life-on-april-30-2025
branch
Description of the changes
Changes
Why
Resolves #1492
Summary by CodeRabbit
Chores
New Features
Refactor