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

[now-client] Add debug logs #2997

Open
wants to merge 14 commits into
base: canary
from

Conversation

@rdev
Copy link
Member

commented Sep 11, 2019

This PR adds extensive debug logging to now-client and enables it in CLI based on the --debug flag

Debug logging works in either of the following two conditions:

  • debug: true is provided in the options object of createDeployment/createLegacyDeployment
  • process.env.NOW_CLIENT_DEBUG environment variable is set
rdev added 2 commits Sep 11, 2019

@rdev rdev requested review from styfle and AndyBitz Sep 11, 2019

@rdev rdev requested review from leo and TooTallNate as code owners Sep 11, 2019

packages/now-client/src/deploy.ts Outdated Show resolved Hide resolved
@AndyBitz
Copy link
Contributor

left a comment

Tests need to pass

@styfle styfle added the now-client label Sep 11, 2019

@styfle styfle changed the title Add `now-client` debug logs [now-client] Add debug logs Sep 11, 2019

@TooTallNate

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

This looks like a good starting point. One part that kind of bothers me is that in general I don't think JS libraries should directly output debugging information, but instead provide a mechanism for the top-level app to consume the libraries debugging information.

I was thinking that now-client could yield objects of type: "debug" when the debugging mode is enabled. Generator functions and yield * could be a "neat" way of accomplishing this while staying DRY:

function createDebug(enabled = false) {
  return async function* debug(log) {
    if (enabled) {
      yield { type: 'debug', payload: log };
    }
  }
}

async function* deploy(opts) {
  const debug = createDebug(opts.debug);

  yield { type: 'initializing' };
  yield* debug('uploading files');
  yield { type: 'ready' };
}

async function main() {
  console.log('deploying without debug');
  for await (const event of deploy({ debug: false })) {
    console.log({ event });
  }
  console.log('done deploying without debug');

  console.log();

  console.log('deploying with debug');
  for await (const event of deploy({ debug: true })) {
    console.log({ event });
  }
  console.log('done deploying with debug');
}

main().catch(console.error);
$ node t
deploying without debug
{ event: { type: 'initializing' } }
{ event: { type: 'ready' } }
done deploying without debug

deploying with debug
{ event: { type: 'initializing' } }
{ event: { type: 'debug', payload: 'uploading files' } }
{ event: { type: 'ready' } }
done deploying with debug

Just an idea.

rdev added 3 commits Sep 17, 2019
@rdev

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

@TooTallNate I was thinking about it but wasn't sure how to neatly accomplish it. TIL about yield*
Will do it the events way 🙏

Edit: Just tried implementing it, it requires quite a sizable rewrite of all major components of now-client
Perhaps it's best if we do it in the future

@codecov-io

This comment has been minimized.

Copy link

commented Sep 17, 2019

Codecov Report

Merging #2997 into canary will decrease coverage by 1.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           canary    #2997      +/-   ##
==========================================
- Coverage   13.46%   12.29%   -1.17%     
==========================================
  Files         267      267              
  Lines       10294    10157     -137     
  Branches     1211     1280      +69     
==========================================
- Hits         1386     1249     -137     
+ Misses       8846     8817      -29     
- Partials       62       91      +29
Impacted Files Coverage Δ
src/util/dev/server.ts 60.69% <0%> (-5.74%) ⬇️
src/util/dev/yarn-installer.ts 85.71% <0%> (-5.03%) ⬇️
src/util/dev/builder.ts 73.07% <0%> (-4.35%) ⬇️
src/util/dev/router.ts 78.57% <0%> (-4.13%) ⬇️
src/commands/logs.js 0% <0%> (ø) ⬆️

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 6bebc49...48825d1. Read the comment docs.

rdev added 2 commits Sep 17, 2019

@rdev rdev requested review from AndyBitz, styfle and TooTallNate Sep 17, 2019

rdev added 3 commits Sep 20, 2019
@styfle
styfle approved these changes Sep 20, 2019

@rdev rdev added the automerge label Sep 20, 2019

@styfle styfle added the now-cli label Sep 20, 2019

Tests are green 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.