Skip to content
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

Core: Add logLevel preset property to filter logging #10370

Merged
merged 26 commits into from
Jun 2, 2020
Merged

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Apr 9, 2020

Issue: #8461

What I did

ADD logLevel preset property (default is info)
ADD global variable injection
ADD filter for lower level log messages
ADD a global method for changing the logLevel at runtime.

@ndelangen ndelangen self-assigned this Apr 9, 2020
@ndelangen ndelangen added this to the 6.0 milestone Apr 9, 2020
@ndelangen ndelangen changed the title ADD logLever preset property && filter lower level log messages ADD logLevel preset property && filter lower level log messages Apr 10, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM but a few questions

addons/links/src/preview.test.js Outdated Show resolved Hide resolved
lib/core/src/server/preview/iframe-webpack.config.js Outdated Show resolved Hide resolved
@shilman shilman changed the title ADD logLevel preset property && filter lower level log messages Core: Add logLevel preset property to filter logging Apr 11, 2020
@ndelangen ndelangen requested a review from stijnkoopal as a code owner April 13, 2020 23:05
@ndelangen
Copy link
Member Author

No idea why puppeteer is all of a sudden not able to find the globals anymore, and use them to extract.

When I run it locally it works.

@shilman shilman added the core label May 2, 2020
ndelangen added 3 commits May 14, 2020 11:04
it's not used anywhere I think, but we're getting quite a few 'invalid cli option' errors,
and this should get rid of them.
debug: (message: any, ...rest: any[]): void => console.debug(message, ...rest),
log: (message: any, ...rest: any[]): void => console.log(message, ...rest),
log: (message: any, ...rest: any[]): void => window.console.log(message, ...rest),
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty surprising, what's up with this?

Copy link
Member Author

@ndelangen ndelangen May 26, 2020

Choose a reason for hiding this comment

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

console.log has no "level" to it. it's not part of console lib.

I could decide to make log equal to info?

lib/client-logger/src/index.ts Outdated Show resolved Hide resolved
@tmeasday
Copy link
Member

@ndelangen I think the most likely situation is that the storybook is throwing an error as it loads in puppeteer and we aren't seeing the error. Try adding:

  this.page.on('console', (msg) => {
    const type = msg.type();
    logger.info({ browserConsole: true, type, ...msg.location() }, msg.text());
  });

Either that or get a build storybook off one of the CI machines (I have one here)

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM does this need documentation somewhere?

@ndelangen
Copy link
Member Author

I added documentation and the ability to set a value for this from the cli

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants