(#1513) Replace `console` with `client-logger` and `node-logger` packages #2347

dangreenisrael · 2017-11-22T02:34:31Z

Issue: #1513

What I did

Added a logger package containing a logger to be use in the browser and another to be used in node.

Note: This is for work in progress. Any feedback is more than welcome.

How to test

Is this testable with jest or storyshots?
Jest

Does this need a new example in the kitchen sink apps?
No

Does this need an update to the documentation?
Yes, it will need to be documented for add-on developers

If your answer is yes to any of these, please make sure to include it in your PR.

Screenshots

Node

Browser

codecov · 2017-11-22T02:55:06Z

Codecov Report

Merging #2347 into release/3.3 will increase coverage by 0.09%.
The diff coverage is 60%.

@@               Coverage Diff               @@
##           release/3.3    #2347      +/-   ##
===============================================
+ Coverage        21.48%   21.58%   +0.09%     
===============================================
  Files              337      339       +2     
  Lines             7098     7107       +9     
  Branches           875      880       +5     
===============================================
+ Hits              1525     1534       +9     
+ Misses            4934     4915      -19     
- Partials           639      658      +19

Impacted Files	Coverage Δ
app/react/src/client/preview/render.js	`0% <ø> (ø)`	⬆️
app/react/src/server/config.js	`0% <ø> (ø)`	⬆️
app/react/src/server/babel_config.js	`76.66% <ø> (ø)`	⬆️
app/react/src/server/index.js	`0% <0%> (ø)`	⬆️
app/react/src/server/build.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/client_api.js	`85.71% <0%> (ø)`	⬆️
lib/client-logger/src/index.js	`100% <100%> (ø)`
lib/node-logger/src/index.js	`100% <100%> (ø)`
app/vue/src/server/config/babel.js	`0% <0%> (-100%)`	⬇️
lib/ui/src/modules/ui/components/layout/usplit.js	`22.58% <0%> (ø)`	⬆️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c5c00c...a34e709. Read the comment docs.

dangreenisrael · 2017-11-22T04:22:48Z

There seems to be some sort of issue with the build step of the integration tests that is happening on circle but not local for me 😕

It ends with

Storybook started on => http://localhost:9009/

Step was canceled

Hypnosphi · 2017-11-22T10:13:31Z

This is for app/react only

Why?

Hypnosphi · 2017-11-22T10:14:27Z

app/react/src/client/preview/render.js

@@ -3,14 +3,13 @@
 import React from 'react';
 import ReactDOM from 'react-dom';
 import { stripIndents } from 'common-tags';
+import logger from 'npmlog';


Does it work on client?

(facepalm) It was late when I was working on this. Thanks @Hypnosphi. I was indeed trying to use a node logger in a browser environment.

dangreenisrael · 2017-11-22T12:42:52Z

I just wanted to make sure that this was what the team wanted, before making the change throughout the ecosystem.

…rowser

dangreenisrael · 2017-11-22T13:22:02Z

@Hypnosphi @ndelangen Do you guys prefer to import npmlog directly into files, or have a storybook-client-logger and storybook-node-logger that we keep in a central location and use throughout the mono-repo?

ndelangen · 2017-11-22T15:59:08Z

@Hypnosphi @ndelangen Do you guys prefer to import npmlog directly into files, or have a storybook-client-logger and storybook-node-logger that we keep in a central location and use throughout the mono-repo?

Sounds like a good thing indeed, if we want to change logger, we can do it in a central location 👍

ndelangen · 2017-11-22T16:01:04Z

@dangreenisrael I usually take some screenshots of the changes just before opening up the PR, so I can post them.

At least for me, it helps visualise and remember what the PR is about, what it's doing.

👍

…in `app/react`

…storybooks/storybook into Replace-console.log-with-npmlog

Hypnosphi · 2017-11-25T09:39:08Z

@dangreenisrael feel free to merge if you think it's ready

Hypnosphi · 2017-11-25T18:04:58Z

As you wish. I think there’s a sense in adding this to all the packages that log anything

dangreenisrael · 2017-11-25T18:06:54Z

@Hypnosphi I think I would prefer to do each platform in its own, smaller PR. I think that keeping PRs small makes things more manageable, and makes it easier to track down bugs if they are introduced.

Hypnosphi · 2017-11-25T18:13:21Z

Will it be a separate PR for each addon and utility package (lib/*) as well?

dangreenisrael · 2017-11-25T18:14:30Z

Not all of the addons use console.log. I will probably do one PR for the each of the platforms, and one PR for the all of the addons . Also, if I run out of time doing this, I want it to be as easy as possible for someone else to takeover.

(#1513) Replace console with npmlog in app/react

e35045f

dangreenisrael force-pushed the Replace-console.log-with-npmlog branch from d682c29 to e35045f Compare November 22, 2017 02:55

dangreenisrael added the ci: do not merge label Nov 22, 2017

Hypnosphi reviewed Nov 22, 2017

View reviewed changes

(#1513) Remove references to npm log from files that are run in the b…

f82ae06

…rowser

Merge branch 'release/3.3' into Replace-console.log-with-npmlog

4085c19

dangreenisrael self-assigned this Nov 22, 2017

dangreenisrael changed the title ~~(#1513) Replace console with npmlog in app/react~~ (#1513) Replace console with npmlog Nov 22, 2017

(#1513) Add logging package to lib/logger

d13c51c

dangreenisrael changed the title ~~(#1513) Replace console with npmlog~~ (#1513) Replace console with logger package Nov 23, 2017

dangreenisrael changed the title ~~(#1513) Replace console with logger package~~ (#1513) [Work in Progress] Replace console with logger package Nov 23, 2017

dangreenisrael and others added 4 commits November 23, 2017 09:30

Merge branch 'release/3.3' into Replace-console.log-with-npmlog

a720e7d

Merge branch 'release/3.3' into Replace-console.log-with-npmlog

0499713

(#1513) Broke up client/node logger packages, implemented new logger …

6619a56

…in `app/react`

Merge branch 'Replace-console.log-with-npmlog' of https://github.com/…

283f6dd

…storybooks/storybook into Replace-console.log-with-npmlog

Hypnosphi approved these changes Nov 25, 2017

View reviewed changes

dangreenisrael changed the title ~~(#1513) [Work in Progress] Replace console with logger package~~ (#1513) Replace console with client-logger and node-logger packages Nov 25, 2017

dangreenisrael added ci: do not merge and removed ci: do not merge labels Nov 25, 2017

dangreenisrael force-pushed the Replace-console.log-with-npmlog branch from b699f7e to 03576fc Compare November 25, 2017 17:13

(#1513) Update to test descriptions

fa16aa8

dangreenisrael force-pushed the Replace-console.log-with-npmlog branch from 2fa1b27 to fa16aa8 Compare November 25, 2017 17:15

Merge branch 'release/3.3' into Replace-console.log-with-npmlog

a34e709

dangreenisrael removed the ci: do not merge label Nov 25, 2017

dangreenisrael merged commit ba115e6 into release/3.3 Nov 25, 2017

dangreenisrael deleted the Replace-console.log-with-npmlog branch November 25, 2017 18:04

This was referenced Jan 16, 2022

[Snyk] Security upgrade marked from 0.3.19 to 4.0.10 Bhanditz/storybook#78

Open

[Snyk] Security upgrade marked from 0.3.19 to 4.0.10 adrianblynch/storybook#161

Open

[Snyk] Security upgrade marked from 0.3.19 to 4.0.10 adrukh/storybook#254

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#1513) Replace `console` with `client-logger` and `node-logger` packages #2347

(#1513) Replace `console` with `client-logger` and `node-logger` packages #2347

dangreenisrael commented Nov 22, 2017 •

edited

Loading

codecov bot commented Nov 22, 2017 •

edited

Loading

dangreenisrael commented Nov 22, 2017 •

edited

Loading

Hypnosphi commented Nov 22, 2017

Hypnosphi Nov 22, 2017

dangreenisrael Nov 22, 2017 •

edited

Loading

dangreenisrael commented Nov 22, 2017

dangreenisrael commented Nov 22, 2017 •

edited

Loading

ndelangen commented Nov 22, 2017

ndelangen commented Nov 22, 2017

Hypnosphi commented Nov 25, 2017

Hypnosphi commented Nov 25, 2017

dangreenisrael commented Nov 25, 2017

Hypnosphi commented Nov 25, 2017 •

edited

Loading

dangreenisrael commented Nov 25, 2017 •

edited

Loading

(#1513) Replace console with client-logger and node-logger packages #2347

(#1513) Replace console with client-logger and node-logger packages #2347

Conversation

dangreenisrael commented Nov 22, 2017 • edited Loading

What I did

How to test

Screenshots

Node

Browser

codecov bot commented Nov 22, 2017 • edited Loading

Codecov Report

dangreenisrael commented Nov 22, 2017 • edited Loading

Hypnosphi commented Nov 22, 2017

Hypnosphi Nov 22, 2017

Choose a reason for hiding this comment

dangreenisrael Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

dangreenisrael commented Nov 22, 2017

dangreenisrael commented Nov 22, 2017 • edited Loading

ndelangen commented Nov 22, 2017

ndelangen commented Nov 22, 2017

Hypnosphi commented Nov 25, 2017

Hypnosphi commented Nov 25, 2017

dangreenisrael commented Nov 25, 2017

Hypnosphi commented Nov 25, 2017 • edited Loading

dangreenisrael commented Nov 25, 2017 • edited Loading

(#1513) Replace `console` with `client-logger` and `node-logger` packages #2347

(#1513) Replace `console` with `client-logger` and `node-logger` packages #2347

dangreenisrael commented Nov 22, 2017 •

edited

Loading

codecov bot commented Nov 22, 2017 •

edited

Loading

dangreenisrael commented Nov 22, 2017 •

edited

Loading

dangreenisrael Nov 22, 2017 •

edited

Loading

dangreenisrael commented Nov 22, 2017 •

edited

Loading

Hypnosphi commented Nov 25, 2017 •

edited

Loading

dangreenisrael commented Nov 25, 2017 •

edited

Loading