Remove js-logger dependency#966
Conversation
🦋 Changeset detectedLatest commit: e733396 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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 |
2b898c3 to
6d97c35
Compare
LucDeCaf
left a comment
There was a problem hiding this comment.
I like the changes outlined here. There's just a few places which seem to be leftovers from an earlier draft, or which didn't get translated as cleanly.
| this.logger.error(`UploadAttachment error for record ${JSON.stringify(record, null, 2)}`); | ||
| this.logger.log({ | ||
| level: LogLevels.error, | ||
| message: `UploadAttachment error for record ${JSON.stringify(record, null, 2)}` |
There was a problem hiding this comment.
We can probably assign the log's error field to the caught error object.
| this.logger.error('Downloads failed:', e); | ||
| this.logger.log({ level: LogLevels.debug, message: 'Finished downloading attachments' }); | ||
| } catch (error) { | ||
| this.logger.log({ level: LogLevels.error, message: 'Downloads failed:', error }); |
There was a problem hiding this comment.
Nit: ":" after "Downloads failed" doesn't really make sense anymore :)
| // Device came online, process attachments immediately | ||
| this.syncStorage().catch((error) => { | ||
| this.logger.error('Error syncing storage on connection:', error); | ||
| this.logger.log({ level: LogLevels.error, message: 'Error syncing storage on connection:', error }); |
| this.logger.log({ | ||
| level: LogLevels.error, | ||
| message: `Could not POST streaming to ${path} - ${res.status} - ${res.statusText}: ${text}` | ||
| }); |
There was a problem hiding this comment.
I think we can assign the log's error to the error created below it before throwing.
| ); | ||
| this.logger.log({ | ||
| level: LogLevels.debug, | ||
| message: `Caught exception when uploading. Upload will retry after a delay. Exception: ${(ex as Error).message}` |
There was a problem hiding this comment.
This is debatable because it's a debug log, but we might want to pass in the error anyways. No strong opinion.
| console.log(...args); | ||
| logMessages.push(util.format(...args)); | ||
| const logger: PowerSyncLogger = { | ||
| log(_level, ...message) { |
There was a problem hiding this comment.
This looks like it's still using the initial PowerSyncLogger.log(level: number, ...message: any[]) syntax.
| logger.error('Failed to parse query:', e); | ||
| handleError(e); | ||
| } catch (error) { | ||
| logger?.log({ level: LogLevels.error, message: 'Failed to parse query:', error }); |
| logger.error('Failed to fetch data:', e); | ||
| handleError(e); | ||
| } catch (error) { | ||
| logger?.log({ level: LogLevels.error, message: 'Failed to fetch data:', error }); |
| logger.error('Failed to parse query:', e); | ||
| handleError(e); | ||
| } catch (error) { | ||
| logger.log({ level: LogLevels.error, message: 'Failed to parse query:', error }); |
As proposed in #948, this removes the
js-loggerdependency in favor of a custom logging interface. The idea is that:levelas a named parameter instead of one method per level) to keep the interface smaller.createBaseLogger. If you want a PowerSync database to log, you pass a logger in. By default, we use an implementation forwarding logs toconsole.log.For the most part, users can keep using the default option and continue to get a default out-of-the-box experience where messages with level
infoand higher are logged to the console with a[PowerSync]prefix. To customize this:PowerSyncLoggerinterface, and pass an instance of that toPowerSyncDatabase. Or, if you want the default logger but with another minimum severity, usecreatePowerSyncLogger.WASQLiteOpenFactorydirectly, one also needs to pass a logger. The web SDK would forward the one from the database by default.logLevelcan be passed toWASQLiteOpenFactory, and messages with that level or higher are forwarded to the main tab.This is a breaking change, which is why this PR has the
v2branch as a target. The idea is to bundle up changes which we can then release at once.