-
Notifications
You must be signed in to change notification settings - Fork 10
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
Winston Logger Adapter #622
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes integrate a Winston logging adapter into the Hive Gateway for Node.js applications. This includes the introduction of a new package with its metadata, the implementation of the logging adapter, and associated unit tests. Additionally, documentation is updated with a section on the "Winston Adapter" that features a TypeScript example, and the dependency version for Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)`packages/**`: In this directory we keep all packages releva...
`**`: For all PRs, we would like to verify that a Linear iss...
⏰ Context from checks skipped due to timeout of 90000ms (25)
🔇 Additional comments (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
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.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
packages/winston/package.json (1)
60-62
: Side Effects Flag and Trailing Line CheckThe
"sideEffects": false
flag is correctly set, indicating that the package is side-effect free for optimal bundling. Also, please verify that no extraneous content (such as a stray line corresponding to"62"
) is present in the actual file, as such content could lead to issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (4)
.changeset/wild-balloons-repair.md
(1 hunks)packages/winston/package.json
(1 hunks)packages/winston/src/index.ts
(1 hunks)packages/winston/tests/winston.spec.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/winston/tests/winston.spec.ts
packages/winston/src/index.ts
packages/winston/package.json
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/winston/tests/winston.spec.ts
packages/winston/src/index.ts
packages/winston/package.json
🪛 Biome (1.9.4)
packages/winston/tests/winston.spec.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 7-7: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
packages/winston/src/index.ts
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
[error] 14-14: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Leaks / Node v23
- GitHub Check: Leaks / Node v22
- GitHub Check: Leaks / Node v20
- GitHub Check: Unit / Bun
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Snapshot / snapshot
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Bundle
- GitHub Check: Unit / Node v23
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Build
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Unit / Node v22
- GitHub Check: Unit / Node v20
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Format
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Unit / Node v18
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
packages/winston/src/index.ts (1)
67-71
: Implementation looks good.
The function seamlessly creates a logger adapter from an existing Winston logger.packages/winston/tests/winston.spec.ts (1)
2-180
: Great test coverage.
These tests are comprehensive and cover a variety of input scenarios, including nested loggers.🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
.changeset/wild-balloons-repair.md (2)
2-2
: Verify the necessity for a major version bump.
The guidelines indicate that major bumps are typically reserved for breaking changes. If this addition is a new feature, consider a minor bump instead.
1-6
: Linear issue reference missing.
Per the guidelines, please ensure a "GW-*" issue is linked in the PR description or clarify if it's not relevant.packages/winston/package.json (10)
5-9
: Repository Metadata ConfigurationThe repository field correctly references the Git repository URL and specifies the monorepo directory
"packages/winston"
. Ensure that this accurately reflects your repository structure.
10-15
: Homepage and Author DetailsThe homepage URL and author metadata (name, email, URL) are clearly defined, providing transparent project information.
16-20
: License, Engines, and Main EntryThe license (MIT), Node.js engine requirement (
>=18.0.0
), and the main entry file ("./dist/index.js"
) are all correctly specified.
34-34
: Type Definitions ReferenceThe
"types"
field correctly points to"./dist/index.d.ts"
, ensuring TypeScript consumers receive the necessary type definitions.
35-37
: Whitelisting Published FilesThe
"files"
array is properly configured to restrict the published package contents to thedist
directory, which is a best practice for clean package distribution.
38-41
: Build and Prepack ScriptsThe defined scripts—using
"pkgroll --clean-dist"
for the build and"yarn build"
for the prepack step—are correctly set up to ensure the package is built before it’s published.
42-45
: Peer Dependencies SpecificationThe peer dependencies for
"graphql"
and"winston"
are specified with version ranges that seem appropriate. This ensures that consumers of the package install compatible versions.
46-50
: Optional Peer Dependency MetadataThe use of
"peerDependenciesMeta"
to mark"@parcel/watcher"
as optional is correctly implemented, providing flexibility for users who might not need that dependency.
51-54
: Runtime DependenciesThe runtime dependencies,
"@graphql-mesh/types"
and"tslib"
, are included with appropriate version constraints. Everything appears standard and well-configured.
55-59
: Development DependenciesThe devDependencies list the required packages for development and testing, including
"graphql"
,"pkgroll"
, and"winston"
. Including"winston"
here, even though it’s already in peerDependencies, helps ensure consistent behavior during development.
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-tools/batch-delegate |
9.0.30-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/batch-execute |
9.0.12-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/delegate |
10.2.12-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-common |
0.0.2-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-graphql-ws |
2.0.2-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-http |
1.2.7-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/federation |
3.1.2-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-runtime |
0.11.0-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.10.0-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/importer |
1.0.1-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/logger-json |
0.0.1-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/logger-winston |
1.0.0-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/hmac-upstream-signature |
1.2.20-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.41-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.29-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.4.13-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitch |
9.4.17-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitching-directives |
3.1.27-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-common |
0.7.29-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http |
0.6.33-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http-callback |
0.5.20-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
1.0.3-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/wrap |
10.0.30-alpha-a3b872e50dfd5c5a4bde6078744593b850e80b1f |
npm ↗︎ unpkg ↗︎ |
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.changeset/wild-balloons-repair.md
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Bun Docker image
- GitHub Check: Node Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Leaks / Node v20
- GitHub Check: Leaks / Node v18
🔇 Additional comments (3)
.changeset/wild-balloons-repair.md (3)
5-7
: Clear Heading and Description
The "Winston Adapter" section title and the accompanying description clearly communicate the integration of the Winston logging library into Hive Gateway for Node.js. This makes the purpose of the changeset immediately obvious to users.
9-11
: Import Statements for Logger Functions
The imports from'winston'
and@graphql-hive/winston
are correctly declared, ensuring that the necessary functions (createLogger
,format
,transports
, andcreateLoggerFromWinston
) are available for use in the snippet.
13-23
: Winston Logger Setup
The code for creating the Winston logger is well-structured. It usesformat.combine
to include both a timestamp and JSON formatting, and initializes a console transport correctly. This adheres to common best practices for configuring a Winston logger.
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
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.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.changeset/wild-balloons-repair.md
(1 hunks)packages/winston/tests/winston.spec.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/winston/tests/winston.spec.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/winston/tests/winston.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Unit / Node v23
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Node v22
- GitHub Check: Leaks / Node v22
- GitHub Check: Leaks / Node v20
- GitHub Check: Unit / Node v20
- GitHub Check: Leaks / Node v18
- GitHub Check: Unit / Node v18
- GitHub Check: Unit / Bun
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Bundle
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Build
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/winston/tests/winston.spec.ts (1)
15-178
: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- All log levels (error, warn, info, debug)
- Basic logging functionality
- Child logger behavior
- Name deduplication
- Nested logger scenarios
.changeset/wild-balloons-repair.md (1)
1-31
: Documentation and issue tracking requirements.The changeset looks good but there are a few requirements to address:
- No Linear issue (GW-*) is mentioned in the PR description
- Documentation in the
console
project might be needed for this new featureLet me check if there's any documentation PR in the console repository:
Please:
- Add the Linear issue reference (GW-*) to the PR description
- Create a follow-up issue to add documentation in the console project if none exists
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for documentation PRs in graphql-hive/console repository gh search prs --repo graphql-hive/console --state open --json title,body,url \ | jq '.[] | select(.title | test("winston|logger"; "i")) | {title, url}'Length of output: 151
Missing Issue Reference & Documentation Follow-Up
The changeset correctly implements the Winston adapter integration, but:
- The PR description is missing a reference to a Linear issue (GW-*).
- No open documentation PR was found in the console repository for this new feature. Please add the Linear issue reference to the PR description and, if needed, create a follow-up issue for updating the console documentation.
8d12b25
to
5bf435c
Compare
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.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/winston/package.json (1)
1-62
: Ensure Changeset and Linear Issue AssociationSince this new package introduces the Winston Logger Adapter functionality, please confirm that a corresponding changeset file has been created with the appropriate semver bump (typically a minor change for new features). Moreover, ensure that a related Linear issue (using the "GW-*" pattern) is referenced in the PR description to provide context and traceability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (4)
.changeset/wild-balloons-repair.md
(1 hunks)packages/winston/package.json
(1 hunks)packages/winston/src/index.ts
(1 hunks)packages/winston/tests/winston.spec.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/winston/src/index.ts
packages/winston/package.json
packages/winston/tests/winston.spec.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/winston/src/index.ts
packages/winston/package.json
packages/winston/tests/winston.spec.ts
🪛 Biome (1.9.4)
packages/winston/src/index.ts
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
[error] 14-14: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Unit / Node v23
- GitHub Check: Unit / Bun
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Node v22
- GitHub Check: Leaks / Node v22
- GitHub Check: Unit / Node v20
- GitHub Check: Leaks / Node v20
- GitHub Check: Unit / Node v18
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Missing peer deps
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Build
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
packages/winston/src/index.ts (3)
10-10
: UseNumber.POSITIVE_INFINITY
instead ofInfinity
.ES2015 and subsequent standards moved some globals into the
Number
namespace for clarity.- .flat(Infinity) + .flat(Number.POSITIVE_INFINITY)🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
14-14
: Avoid reassigning function parameters.Reassigning
arg
may confuse readers. Use a local variable instead.- arg = JSON.parse(arg); + const parsedArg = JSON.parse(arg); + return parsedArg;🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
27-65
: Consider supporting Winston's additional log levels.Winston provides extra log levels like
verbose
andsilly
. If extended logging is desired, consider adding them.packages/winston/tests/winston.spec.ts (1)
8-14
: Use a more robust callback tracking mechanism.Storing the last callback with
lastCallback
could be fragile in concurrent or async scenarios. Use a more dependable approach, such as a queue of callbacks or a promise-based method..changeset/wild-balloons-repair.md (3)
1-3
: Verify whether a major version bump is necessary.There's an existing comment about ensuring the major version is correct for
@graphql-hive/winston
. Please confirm that this reflects a breaking change or adopt a minor/patch version if it doesn't.
1-1
: Missing Linear issue reference.No "GW-" issue is mentioned in the PR description. If this PR is part of a Linear board task, please link the corresponding "GW-" issue here.
9-30
: Consider adding an error handling example.An example of a logged error would showcase best practices for diagnosing and reporting failures.
packages/winston/package.json (9)
5-10
: Repository and Homepage AccuracyThe
"repository"
field and"homepage"
URL are correctly specified. Please verify that the homepage URL directs users to the appropriate documentation for the new Winston Logger Adapter. This can help ensure consistency with your project’s branding and documentation.
11-15
: Author Information is ClearThe author details are well defined and consistent with the project’s branding. No adjustments are needed here.
16-19
: License and Node Engine RequirementsThe license and Node.js engine requirements are appropriately set. Ensure that the engine version (
>=18.0.0
) aligns with the rest of your packages.
20-33
: Dual Module Support in the Exports FieldThe
"exports"
field is structured to support both CommonJS and ES module systems, including type definitions. Please confirm that using the custom type declaration file ("./dist/index.d.cts"
) for CommonJS usage is intentional and fully supported by your toolchain.
34-37
: Types and Files ConfigurationThe configuration for
"types"
and the"files"
array is correct and will help ensure that only the necessary distribution files are included. This setup is appropriate for clean package distribution.
38-41
: Build and Prepack ScriptsThe build and prepack scripts (using
pkgroll --clean-dist
andyarn build
) follow common practices for preparing a package. Verify that the output in thedist
folder is as expected.
42-50
: Peer Dependencies and Optional MetaThe
"peerDependencies"
field correctly defines the required external packages, and"peerDependenciesMeta"
appropriately marks@parcel/watcher
as optional. This is in line with best practices.
51-59
: Dependencies ConsistencyThe versions for
"dependencies"
and"devDependencies"
are specified with appropriate version ranges. One point to verify is that the version ofgraphql
in the dev dependencies (i.e.,"16.10.0"
) aligns well with the range defined in the peer dependencies ("^15.9.0 || ^16.9.0"
). Ensure there’s no unintended version mismatch during development.
60-60
: Side Effects FlagMarking
"sideEffects"
asfalse
is a good practice for tree shaking and indicates that the module does not produce side effects upon import. This configuration is correct.
5bf435c
to
78ee389
Compare
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.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/winston/package.json (1)
1-62
: 🧹 Nitpick (assertive)Consider Adding a Package Description
While optional, including a"description"
field in the package metadata can improve discoverability on registries like npm. A brief summary of the package functionality could be valuable for users.
♻️ Duplicate comments (1)
packages/winston/package.json (1)
1-3
: 🛠️ Refactor suggestionUpdate Package Version for Alpha Release
The current version is set to"0.0.0"
, which does not reflect the alpha release status described in the PR. Please update this to an appropriate pre-release identifier (for example,"1.0.0-alpha-8d12b253f742b70368d12b7fd35b718a3d8aa17a"
) to clearly signal its pre-release state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (4)
.changeset/wild-balloons-repair.md
(1 hunks)packages/winston/package.json
(1 hunks)packages/winston/src/index.ts
(1 hunks)packages/winston/tests/winston.spec.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/winston/tests/winston.spec.ts
packages/winston/package.json
packages/winston/src/index.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/winston/tests/winston.spec.ts
packages/winston/package.json
packages/winston/src/index.ts
🪛 Biome (1.9.4)
packages/winston/src/index.ts
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
[error] 14-14: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Unit / Node v23
- GitHub Check: Leaks / Node v20
- GitHub Check: Snapshot / snapshot
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Unit / Bun
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Unit / Node v22
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Unit / Node v20
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Unit / Node v18
- GitHub Check: Build
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
packages/winston/src/index.ts (4)
10-10
: Use Number.POSITIVE_INFINITY instead of Infinity.This recommendation was already provided in a past review comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
14-14
: Avoid reassigning function parameters.This suggestion was already provided in a past review comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
35-59
: Consider supporting Winston’s other log levels.This feedback was already provided in a past review comment.
1-72
: No reference to a linked Linear issue (GW-*) detected.According to the guidelines, please confirm whether a Linear issue (GW-*) should be associated with this PR.
🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
[error] 14-14: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
packages/winston/tests/winston.spec.ts (1)
8-14
: Consider using a more robust callback tracking mechanism.This suggestion was already provided in a past review comment.
.changeset/wild-balloons-repair.md (1)
1-3
: Validate the semver bump for @graphql-hive/winston.The file marks a major version, but if this feature is newly introduced and not breaking existing usage, changing it to a minor version might be more appropriate.
packages/winston/package.json (8)
21-31
: Review Exports Field Configuration
The"exports"
section appropriately supports both CommonJS and ES modules, including specific type definition paths. Please verify that using the custom declaration file ("./dist/index.d.cts"
) under the"require"
entry is supported by your toolchain, as intended.
35-37
: Files Inclusion
The"files"
array restricts the published package to thedist
directory. This is a good practice to minimize the package size and ensure only the built artifacts are published.
38-41
: Build & Prepack Scripts
The build and prepack scripts usingpkgroll --clean-dist
andyarn build
are standard and align with expectations. Make sure that the build output indist
is as expected before publishing.
42-45
: Peer Dependencies Specification
The peer dependencies forgraphql
andwinston
are correctly specified to ensure compatibility with downstream projects.
46-50
: Optional Peer Dependency Configuration
ThepeerDependenciesMeta
entry for"@parcel/watcher"
correctly marks it as optional. This configuration is appropriate.
51-54
: Dependencies Configuration
The dependencies (@graphql-mesh/types
andtslib
) are clearly defined and meet the package requirements.
55-59
: Dev Dependencies
The dev dependencies, includinggraphql
,pkgroll
, andwinston
, are set up for proper testing and development. This configuration looks correct.
60-60
: Side Effects Flag
Setting"sideEffects"
tofalse
is a good practice for optimizing bundling via tree-shaking.
Sample logs from prod testing:
Comment on prod log:
Locally with DEBUG=1 there are some issues, sample:
Comment on those
|
78ee389
to
dd844e6
Compare
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.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/winston/package.json (1)
1-4
:⚠️ Potential issueUpdate package version for alpha release.
The package version is set to "0.0.0" but should reflect its alpha status as mentioned in the PR description.
- "version": "0.0.0", + "version": "1.0.0-alpha-78ee389cb41e09aa27b4d57748634f609664aa63"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (4)
.changeset/wild-balloons-repair.md
(1 hunks)packages/winston/package.json
(1 hunks)packages/winston/src/index.ts
(1 hunks)packages/winston/tests/winston.spec.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/winston/package.json
packages/winston/src/index.ts
packages/winston/tests/winston.spec.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/winston/package.json
packages/winston/src/index.ts
packages/winston/tests/winston.spec.ts
🪛 Biome (1.9.4)
packages/winston/src/index.ts
[error] 11-11: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
[error] 15-15: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Mend Security Check
🔇 Additional comments (11)
packages/winston/src/index.ts (3)
11-11
: UseNumber.POSITIVE_INFINITY
instead ofInfinity
.This comment duplicates a past review note about using
Number.POSITIVE_INFINITY
for clarity and consistency in ES2015.🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
15-15
: Avoid reassigning function parameters.This comment duplicates a past review note about using a local variable instead of reassigning the function parameter
arg
. Reassigning parameters can be confusing.🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
28-28
: Verify theDisposable
import/export.The
WinstonLoggerAdapter
class implementsDisposable
, but it's unclear if'@whatwg-node/disposablestack'
exports it. Please confirm or update the import.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Searching for Disposable export definition in '@whatwg-node/disposablestack' fd disposablestack | xargs cat | grep "export.*Disposable"Length of output: 60
Based on our investigation, there is no evidence in the repository that the package
@whatwg-node/disposablestack
exports a type or interface namedDisposable
. Our initial grep for a matching export pattern returned no output, and a subsequent case‐insensitive search for any export of “Disposable” also yielded no findings.It appears that the implementation of
class WinstonLoggerAdapter implements MeshLogger, Disposable {is expecting a
Disposable
export that isn’t present in the referenced module. Please verify with the dependency’s documentation or source code to determine whether:
- The package is indeed meant to export a
Disposable
type, or- The import should be updated (or replaced with a locally defined type) to correctly match the intended behavior.
If the dependency truly does not export
Disposable
, you should update the import or the implementation accordingly.packages/winston/tests/winston.spec.ts (2)
8-14
: Consider using a more robust callback handling approach.This comment duplicates a past suggestion to reduce fragility by employing a queue or promise-based mechanism instead of reassigning
lastCallback
for asynchronous scenarios.
29-38
: Extract repeated test data into a shared fixture.This duplicates a past suggestion. The
testData
array is reused for multiple test cases; consider placing it in a shared constant to improve maintainability.Also applies to: 67-76, 107-116, 147-156
.changeset/wild-balloons-repair.md (3)
1-3
: Confirm correctness of major version bump.This duplicates a past remark questioning the need for a major release here. Ensure it aligns with the changeset tooling and semver guidelines.
9-30
: Consider providing an error handling example.This duplicates a past suggestion to showcase error-logging usage in addition to the basic setup demonstrated.
5-30
: Ensure documentation updates in the console project.This duplicates a past remark reminding you to add or confirm documentation in the
graphql-hive/console
repository, or create a follow-up issue if missing.packages/winston/package.json (3)
21-33
: Verify the exports field configuration.The exports field is well-structured for dual module support (CommonJS and ESM). Please verify that using the custom declaration file "./dist/index.d.cts" under "require" is supported by your tooling.
51-55
: Update tslib dependency.The tslib dependency might not be the latest version. Let's verify the latest stable version.
✅ Verification successful
🌐 Web query:
What is the latest stable version of tslib npm package?
💡 Result:
The latest stable version of the
tslib
npm package is 2.8.1, released on October 31, 2024[1][3][7]. This version includes fixes for helper functions and workflow improvements[7].For installation:
npm install tslibor with yarn:
yarn add tslibThe package is actively maintained by Microsoft, with regular updates and a healthy release cadence[2][7].
Citations:
- 1: https://www.npmjs.com/package/tslib
- 2: https://snyk.io/advisor/npm-package/tslib
- 3: https://www.npmjs.com/package/tslib?activeTab=versions
- 4: https://www.npmjs.com/package/tslib/v/1.6.1?activeTab=versions
- 5: Cannot find module 'tslib' taskforcesh/bullmq#65
- 6: https://cloudsmith.com/navigator/npm/tslib
- 7: https://github.com/microsoft/tslib/releases
- 8: https://nuget.qite.be/feeds/NPM/tslib/versions
- 9: https://www.npmjs.com/package/@beenotung/tslib
- 10: https://www.npmjs.com/package/tslib/v/1.3.0
Action: No update required for tslib dependency
The tslib version in the file (
^2.8.1
) is already set to the latest stable release.
- File:
packages/winston/package.json
(Lines 51-55)
42-45
: Verify Winston peer dependency version.The Winston peer dependency is set to "^3.17.0". Let's verify if this is the latest stable version and if it includes all required features.
✅ Verification successful
🌐 Web query:
What is the latest stable version of winston npm package?
💡 Result:
The latest stable version of the winston npm package is v3.17.0, released on November 9, 2024[7][3]. This version includes updates such as:
- Dependency bumps for
logform
andwinston-transport
- Security fixes for Node.js 18 compatibility
- New transport integrations (e.g., Parseable transport)[7]
Key details from recent releases:
Version Release Date Notable Changes 3.17.0 2024-11-09 Updated dependencies and transport improvements[7][3] 3.16.0 2024-11-01 Maintenance updates 3.15.0 2024-10-04 Bug fixes and documentation improvements The package maintains MIT licensing and weighs ~271 kB with 11 dependencies[3]. For production use with Elasticsearch or Google Cloud integrations, compatible versions include:
winston-elasticsearch@0.19.0
[4]@google-cloud/logging-winston@6.0.0
[6][9]Install via npm:
npm install winston@3.17.0Citations:
- 1: https://github.com/winstonjs/winston/blob/master/CHANGELOG.md
- 2: https://snyk.io/node-js/winston
- 3: https://www.jsdocs.io/package/winston
- 4: https://www.npmjs.com/package/winston-elasticsearch
- 5: https://www.npmjs.com/package/nest-winston
- 6: https://www.npmjs.com/package/@google-cloud/logging-winston
- 7: https://github.com/winstonjs/winston/releases
- 8: https://betterstack.com/community/guides/logging/how-to-install-setup-and-use-winston-and-morgan-to-log-node-js-applications/
- 9: https://cloud.google.com/nodejs/docs/reference/logging-winston/latest/
- 10: https://www.npmjs.com/package/winston-transport
Winston Peer Dependency Verified:
The peer dependency is set to "^3.17.0", which matches the latest stable version of winston according to the web query. No changes required.
dd844e6
to
89f930e
Compare
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (5)
.changeset/wild-balloons-repair.md
(1 hunks)packages/logger-winston/package.json
(1 hunks)packages/logger-winston/src/index.ts
(1 hunks)packages/logger-winston/tests/winston.spec.ts
(1 hunks)tsconfig.json
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
tsconfig.json
packages/logger-winston/tests/winston.spec.ts
packages/logger-winston/package.json
packages/logger-winston/src/index.ts
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/logger-winston/tests/winston.spec.ts
packages/logger-winston/package.json
packages/logger-winston/src/index.ts
🪛 Biome (1.9.4)
packages/logger-winston/src/index.ts
[error] 23-23: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 35-35: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
[error] 13-13: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 16-16: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 25-25: This let declares a variable that is only assigned once.
'messageArg' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 80-80: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/logger-winston/src/index.ts (1)
95-99
: Looks good.
This helper cleanly constructs aWinstonLoggerAdapter
from an existing Winston logger.packages/logger-winston/tests/winston.spec.ts (1)
6-146
: Tests are comprehensive.
All logging levels, child loggers, and deduplication scenarios appear well-covered. Great job verifying output via JSON parsing. No critical issues found..changeset/wild-balloons-repair.md (3)
2-2
: Check if “major” bump is necessary.This package is newly introduced, so a
minor
bump often suffices unless you’re seeing a breaking change across the ecosystem. Also, ensure this version notation aligns with the changeset tooling requirements.
5-30
: Confirm or create corresponding documentation in the console.As this PR adds new functionality, please ensure the documentation is updated in the
graphql-hive/console
repository or create a follow-up issue if needed.
1-31
: Link a relevant Linear issue or create one.No reference to “GW-*” is found in this PR. Please update the PR description or changeset with a reference to the appropriate Linear ticket, or create one if necessary.
packages/logger-winston/package.json (3)
2-19
: LGTM! Package metadata and configuration look good.The package metadata is well-configured with:
- Appropriate name and repository information
- Node.js engine requirement >=18.0.0
- Proper licensing and author information
20-37
: LGTM! Module configuration is properly set up.The package correctly:
- Provides both CommonJS and ES module formats
- Includes TypeScript type definitions
- Specifies the dist directory in files array
42-60
: Verify the winston peer dependency version.The winston peer dependency is set to
^3.17.0
. Let's verify this is the latest stable version to ensure we're not missing any important features or security fixes.✅ Verification successful
🌐 Web query:
What is the latest stable version of winston?
💡 Result:
The latest stable version of winston is 3.17.0, released on November 9, 2024[2][5]. This release includes updates such as dependency bumps for
logform
andwinston-transport
, along with maintenance changes like CI autotesting and codebase improvements[2].Key details from recent releases:
- v3.17.0 (Nov 2024):
- Dependency updates (
logform
,winston-transport@4.x
)- Maintenance fixes and CI improvements[2]
- v3.16.x – v3.13.x (2024):
- Node.js compatibility adjustments
- Security patches and transport enhancements[2][4]
The npm registry confirms this version as current[5], while earlier sources like JS LibHunt (listing v3.8.1)[1] are outdated by comparison to GitHub’s release history[2] and npm’s package data[5].
Citations:
- 1: https://js.libhunt.com/winston-latest-version
- 2: https://github.com/winstonjs/winston/releases
- 3: https://www.npmjs.com/package/winston-elasticsearch
- 4: https://github.com/winstonjs/winston/blob/master/CHANGELOG.md
- 5: https://www.npmjs.com/package/winston
- 6: https://www.elastic.co/guide/en/ecs-logging/nodejs/current/winston.html
- 7: https://cloud.google.com/nodejs/docs/reference/logging-winston/latest/
- 8: https://classic.yarnpkg.com/en/package/winston
Winston Peer Dependency Verified
The winston peer dependency is set to
^3.17.0
, which matches the latest stable version as confirmed by the web query. No update is needed.tsconfig.json (1)
62-63
: LGTM! Path mapping is correctly configured.The new path mapping for
@graphql-hive/logger-winston
follows the existing pattern and maintains alphabetical order.
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.
Actionable comments posted: 1
♻️ Duplicate comments (6)
packages/logger-winston/src/index.ts (6)
10-10
: 🧹 Nitpick (assertive)Use
Number.POSITIVE_INFINITY
for consistency.ES2015 moved some globals into the
Number
namespace. Replacing.flat(Infinity)
with.flat(Number.POSITIVE_INFINITY)
aligns with modern JavaScript standards.- .flat(Infinity) + .flat(Number.POSITIVE_INFINITY)🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.(lint/style/useNumberNamespace)
13-16
: 🧹 Nitpick (assertive)Avoid reassigning the function parameter inside the callback.
Updating function parameters is confusing. Instead, store the updated value in a new local variable.
- .flatMap((messageArg) => { - if (typeof messageArg === 'function') { - messageArg = messageArg(); - } - if (messageArg?.toJSON) { - messageArg = messageArg.toJSON(); - } - ... + .flatMap((originalArg) => { + let localArg = typeof originalArg === 'function' ? originalArg() : originalArg; + if (localArg?.toJSON) { + localArg = localArg.toJSON(); + } + ...🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 16-16: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
23-23
: 🧹 Nitpick (assertive)Remove the trivially inferred type annotation.
Declaring
message
asstring
is redundant since its initializer already infers the type.- let message: string = ''; + let message = '';🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
25-25
: 🧹 Nitpick (assertive)Use
const
instead oflet
where the value is never reassigned.This improves code clarity and enforces immutability.
-for (let messageArg of flattenedMessageArgs) { +for (const messageArg of flattenedMessageArgs) {🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: This let declares a variable that is only assigned once.
'messageArg' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
35-35
: 🧹 Nitpick (assertive)Use a template literal for string concatenation.
Template literals are more readable and recommended over
+
.- message = message ? message + ', ' + messageArg : messageArg; + message = message ? `${message}, ${messageArg}` : `${messageArg}`;🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
80-80
: 🧹 Nitpick (assertive)Avoid reassigning the
nameOrMeta
parameter in thechild
method.Use a local variable to improve clarity, as repeatedly changing the parameter can be confusing.
if (typeof nameOrMeta === 'string') { - nameOrMeta = { + const newMeta = { name: this.name ? this.name.includes(nameOrMeta) ? this.name : `${this.name}, ${nameOrMeta}` : nameOrMeta, }; return new WinstonLoggerAdapter( - this.winstonLogger.child(nameOrMeta), + this.winstonLogger.child(newMeta), { ...this.meta, - ...nameOrMeta, + ...newMeta, } ); }🧰 Tools
🪛 Biome (1.9.4)
[error] 80-80: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (1)
packages/logger-winston/src/index.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**
: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changeset
with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminor
semver change. When a bug is fixed,patch
needs to be used. Themajor
bump is used for a PR that has breaking changes.
packages/logger-winston/src/index.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**
: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
console
project.
Use the global knowledge feature to search for PRs ingraphql-hive/console
repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/logger-winston/src/index.ts
🪛 Biome (1.9.4)
packages/logger-winston/src/index.ts
[error] 23-23: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 35-35: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 10-10: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
[error] 13-13: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 16-16: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 25-25: This let declares a variable that is only assigned once.
'messageArg' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 80-80: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 10 items
Winston Adapter
Now you can integrate Winston into Hive Gateway on Node.js
In order to try it now, install the alpha of
@graphql-hive/gateway
and@graphql-hive/winston
below; #622 (comment)Related GW-163