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

feat: add time and input #80

Merged
merged 2 commits into from
Apr 15, 2024
Merged

feat: add time and input #80

merged 2 commits into from
Apr 15, 2024

Conversation

lukekarrys
Copy link
Contributor

This adds top-level time and input namespaces.

time is used to facilitate starting/ending timers including the ability to pass in a sync or async callback which will be timed. This is a pattern we already used in npm/cli and can be replaced with this code from proc-log instead.

input is used to send events indicating user input is being read. There are two ways to do this: start and end functions or a read function which will return a promise and emit the promise's resolvers to the consumer.

This also adds a KEYS to each namespace which is an enum for getting the levels by name. In npm/cli our consumer creates these with log.LEVELS.reduce((a, k) => (a[k] = k, a), {}) so that we don't need to type any string names in our handelrs.

@lukekarrys lukekarrys requested a review from a team as a code owner April 14, 2024 18:10
})
process.emit('input', 'read', resolve, reject, ...args)
return promise
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I opted for two different ways to do input is in some specialized producers (eg libnpmexec) we will want to use input.read which will give us more control and also be a breaking change.

In more generic consumers we can just wrap the current code in input.start() and input.end(). Our consumer will be able to listen and respond to these events, but all others can ignore these events so it won't be a breaking change.

t.match(args.slice(0, 2), [Function, Function], 'input gets resolvers')
t.same(args.slice(2), [1, 'two', [3], { 4: 4 }], 'got expected args')
break
default:
Copy link
Member

Choose a reason for hiding this comment

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

What about log.pause and log.resume? We've only ever documented that those take no args themselves. Should we make that a reality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that seems like a good idea now that we have separate functions for each method.

test/index.js Outdated Show resolved Hide resolved

**consumer.js**
```js
const { read } = require('read')
Copy link
Member

Choose a reason for hiding this comment

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

@wraithgar
Copy link
Member

Maybe I'll see it when I review again tomorrow but I don't understand the function of KEYS

@wraithgar
Copy link
Member

This question applies to log.pause/unpause and input.start/end. Whose concern should it be if multiple starts/pauses are called and then multiple unpause/ends? How is a consumer to know when ALL interested parties have indicated that logs should unpause or input has truly ended? Obviously if two asynchronous operations are trying to do input they'll inevitably collide with each other so perhaps that's not a concern to solve. This is thinking out loud at this point.

@lukekarrys
Copy link
Contributor Author

Maybe I'll see it when I review again tomorrow but I don't understand the function of KEYS

It is only there to avoid having to manually type strings in consumers. Here's a diff of what you would replace if KEYS were exported.

+ const { output } = require('proc-log')

 process.on('output', (level, ...args) => {
   switch (level) {
-    case 'standard':
+    case output.KEYS.standard:
       console.log(args)
       break
-    case 'error'
+    case output.KEYS.error:
       console.error(args)
       break
   }
 })

@wraithgar
Copy link
Member

It is only there to avoid having to manually type strings in consumers

You still have to know and type the indices to KEYS, I don't get the difference. I'm willing to accept this is just me though.

@lukekarrys
Copy link
Contributor Author

How is a consumer to know when ALL interested parties have indicated that logs should unpause or input has truly ended?

This is why I opted for time.start/end(name) to take a timer name. Multiple timers can be run and it's up to the consumer to handle all of them simultaneously. This is how lib/utils/timers.js in npm/cli works currently.

input.start/end/read() don't take a name argument since there is only one interface for user input. If there are multiple producers nested which are emitting input events, there will be other problems. So I didn't think it should be encouraged to try and namespace input events. Should this be added to input as an escape hatch for complex terminal UI scenarios I'm not envisioning?

@wraithgar
Copy link
Member

Should this be added to input as an escape hatch for complex terminal UI scenarios I'm not envisioning?

No, you thought about this more than I did already. This is find as-is.

wraithgar
wraithgar previously approved these changes Apr 15, 2024
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

If you want to tweak log.pause and log.resume you still can, but I didn't want to block on that.

@wraithgar
Copy link
Member

My remaining 3 comments (readline, log.pause, KEYS) are just those: comments. None of them are blockers but you can act on them if you choose.

@lukekarrys
Copy link
Contributor Author

@wraithgar Pushed a commit removing args from log.pause() and `log.resume(). Leaving the rest as-is.

@lukekarrys lukekarrys merged commit 4832b21 into main Apr 15, 2024
26 checks passed
@lukekarrys lukekarrys deleted the lk/timers-and-read branch April 15, 2024 16:01
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
lukekarrys pushed a commit that referenced this pull request Apr 15, 2024
🤖 I have created a release *beep* *boop*
---


## [4.1.0](v4.0.0...v4.1.0)
(2024-04-15)

### Features

*
[`e00086e`](e00086e)
[#80](#80) add timers and read
(@lukekarrys)

### Bug Fixes

*
[`4832b21`](4832b21)
[#80](#80) remove args from log
pause/resume (@lukekarrys)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants