Skip to content

[CON-76] Add logger param to some libs functions that CN uses - #2976

Merged
jonaylor89 merged 34 commits into
masterfrom
jn-con-76
May 4, 2022
Merged

[CON-76] Add logger param to some libs functions that CN uses#2976
jonaylor89 merged 34 commits into
masterfrom
jn-con-76

Conversation

@jonaylor89

@jonaylor89jonaylor89 commented Apr 27, 2022

Copy link
Copy Markdown
Contributor

Description

Allows for an optional logger to be passed into libs and passed throughout the services with the default being to use console

Tests

  • npm run test in libs/
  • A up with no errors

How will this change be monitored? Are there sufficient logs?

All loggers default to console so if when debugging, the way to monitor if something is hooked up correctly is if it's logger is console when it shouldn't be.

@jonaylor89jonaylor89 added javascript Pull requests that update Javascript code libs content-node Content Node (previously known as Creator Node) labels Apr 27, 2022
@jonaylor89jonaylor89 changed the title [CON-76] Add logger param to some libs functions that CN uses (WIP)[CON-76] Add logger param to some libs functions that CN usesApr 28, 2022

@SidSethiSidSethi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!
lets get libs CI tests and maddog tests passing to make sure we don't introduce any regressions

Comment threadcreator-node/src/serviceRegistry.js Outdated
Comment threadlibs/src/services/contracts/ContractClient.js Outdated
@SidSethi
SidSethi requested a review from dylanjeffersMay 2, 2022 20:48
@SidSethi

Copy link
Copy Markdown
Contributor

added @dylanjeffers as a reviewer for typescript 🙂

@dylanjeffersdylanjeffers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! one callout on the logger method return types

/**
* Write a 'log' level log.
*/
log: (message: any, ...optionalParams: any[]) => any

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type void instead of any i believe? basically means doesn't return anything and is a pure side-effect

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any loggers that do return something from log()? Is sounds weird but I put any just to cover everything. I can change it if I need to

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think so, but any is fine for now too, no worries

@SidSethiSidSethi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving - if you want to make the changes from code reviews go for it, i'm good with you merging either way

just make sure the tests pass 🙂

@SidSethiSidSethi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per discussion, removing approval since we need to separate out CN / libs changes into separate prs
my bad for not calling this out sooner

@jonaylor89

Copy link
Copy Markdown
ContributorAuthor

per discussion, removing approval since we need to separate out CN / libs changes into separate prs my bad for not calling this out sooner

Done

@jonaylor89
jonaylor89 requested a review from SidSethiMay 4, 2022 12:52

@SidSethiSidSethi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - tagging @dylanjeffers in case you want to take a look again at the ts changes

@dylanjeffersdylanjeffers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

content-nodeContent Node (previously known as Creator Node)javascriptPull requests that update Javascript codelibssize/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jonaylor89@SidSethi@dylanjeffers