fix: replace direct console.* calls with windowLogger in window package#1594
fix: replace direct console.* calls with windowLogger in window package#1594devin-ai-integration[bot] wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: fa67bad The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Original prompt from Guillecan you determine the issue and propose a PR in case it's feasible to maintain the silent logs flag in every part of our sdks? |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
| const { environment, projectId } = validationResult; | ||
| windowLogger.init({ | ||
| packageName: "@crossmint/client-sdk-window", | ||
| environment, | ||
| projectId, |
There was a problem hiding this comment.
packageVersion is missing from init params
The wallets package's initWalletsLogger passes both packageName and packageVersion to logger.init() (packages/wallets/src/logger/init.ts:30-31). This implementation should follow the same pattern for consistency and better log traceability.
| const { environment, projectId } = validationResult; | |
| windowLogger.init({ | |
| packageName: "@crossmint/client-sdk-window", | |
| environment, | |
| projectId, | |
| const { environment, projectId } = validationResult; | |
| windowLogger.init({ | |
| packageName: "@crossmint/client-sdk-window", | |
| packageVersion: "1.0.9", // Consider importing from package.json via a constants file | |
| environment, | |
| projectId, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/window/src/logger.ts
Line: 16-20
Comment:
`packageVersion` is missing from init params
The wallets package's `initWalletsLogger` passes both `packageName` and `packageVersion` to logger.init() (packages/wallets/src/logger/init.ts:30-31). This implementation should follow the same pattern for consistency and better log traceability.
```suggestion
const { environment, projectId } = validationResult;
windowLogger.init({
packageName: "@crossmint/client-sdk-window",
packageVersion: "1.0.9", // Consider importing from package.json via a constants file
environment,
projectId,
});
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const platform = detectPlatform(); | ||
| if (platform === "browser") { | ||
| const sink = new BrowserDatadogSink(environment); | ||
| windowLogger.addSink(sink); | ||
| } else if (platform === "server") { | ||
| const sink = new ServerDatadogSink(environment); | ||
| windowLogger.addSink(sink); |
There was a problem hiding this comment.
use switch statement for consistency with wallets package pattern
The wallets package's initWalletsLogger uses a switch statement for platform handling (packages/wallets/src/logger/init.ts:38-61), which is more consistent and makes it easier to add future platforms like react-native.
| const platform = detectPlatform(); | |
| if (platform === "browser") { | |
| const sink = new BrowserDatadogSink(environment); | |
| windowLogger.addSink(sink); | |
| } else if (platform === "server") { | |
| const sink = new ServerDatadogSink(environment); | |
| windowLogger.addSink(sink); | |
| const platform = detectPlatform(); | |
| switch (platform) { | |
| case "browser": { | |
| const sink = new BrowserDatadogSink(environment); | |
| windowLogger.addSink(sink); | |
| break; | |
| } | |
| case "server": { | |
| const sink = new ServerDatadogSink(environment); | |
| windowLogger.addSink(sink); | |
| break; | |
| } | |
| default: { | |
| // Unknown platform - only use console sink | |
| break; | |
| } | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/window/src/logger.ts
Line: 22-29
Comment:
use switch statement for consistency with wallets package pattern
The wallets package's `initWalletsLogger` uses a switch statement for platform handling (packages/wallets/src/logger/init.ts:38-61), which is more consistent and makes it easier to add future platforms like react-native.
```suggestion
const platform = detectPlatform();
switch (platform) {
case "browser": {
const sink = new BrowserDatadogSink(environment);
windowLogger.addSink(sink);
break;
}
case "server": {
const sink = new ServerDatadogSink(environment);
windowLogger.addSink(sink);
break;
}
default: {
// Unknown platform - only use console sink
break;
}
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| export function initWindowLogger(apiKey: string): void { | ||
| const validationResult = validateAPIKey(apiKey); | ||
| if (!validationResult.isValid) { | ||
| return; |
There was a problem hiding this comment.
silently returning on invalid API key is inconsistent with the wallets package pattern
The wallets package's initWalletsLogger at packages/wallets/src/logger/init.ts:24-26 throws an error when the API key is invalid, while this implementation silently returns. This inconsistency could make debugging harder since users won't know why logging isn't working. Consider throwing an error with the validation message instead.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/window/src/logger.ts
Line: 11-14
Comment:
silently returning on invalid API key is inconsistent with the wallets package pattern
The wallets package's `initWalletsLogger` at packages/wallets/src/logger/init.ts:24-26 throws an error when the API key is invalid, while this implementation silently returns. This inconsistency could make debugging harder since users won't know why logging isn't working. Consider throwing an error with the validation message instead.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Description
Replaces all 14 direct
console.info()/console.error()calls in@crossmint/client-sdk-windowwith a newwindowLogger(SdkLoggerinstance), so that these logs respect theconsoleLogLevelsetting (e.g."silent").Previously, the EventEmitter, HandshakeParent, and HandshakeChild classes logged directly to the console, bypassing the SDK logger system entirely.
This is part of a series of per-package PRs to route all SDK console output through
SdkLogger. See related PRs: #1592 (wallets), #1593 (auth).Files changed:
src/logger.ts— new file; createswindowLoggerinstance andinitWindowLogger(apiKey)initializersrc/EventEmitter.ts— 10 console calls → windowLoggersrc/handshake/Parent.ts— 2 console calls → windowLoggersrc/handshake/Child.ts— 2 console calls → windowLoggersrc/index.ts— exportswindowLoggerandinitWindowLoggerpackage.json— adds@crossmint/common-sdk-baseworkspace dependencyinitWindowLoggeris exported but never called within this package. A consuming package (e.g. react-base provider) must callinitWindowLogger(apiKey)for the logger to be fully initialized. Until then,SdkLoggerfalls back to rawconsole.*— meaning the silent behavior won't take effect for window logs until the init call is wired up upstream.@crossmint/common-sdk-baseis added to this previously minimal package (only hadzod). Verify this is acceptable for bundle size.Test plan
pnpm lint)console.*calls inpackages/client/window/src/Package updates
@crossmint/client-sdk-window: patch (changeset added)Link to Devin run: https://crossmint.devinenterprise.com/sessions/3befe224fd2b472594dafcd3fc100e7e
Requested by: @xmint-guille