Skip to content

chore: convert src/logging.{js,ts} #1907

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bkeepers
Copy link
Contributor

As a way of getting familiar with the SignalK internals, I'm working on converting some of the remaining code to TypeScript. This converts src/logging.js to TypeScript.

@tkurki
Copy link
Member

tkurki commented Mar 25, 2025

Would it make sense to compile a single PR instead of PR per each file? Do you have a plan of attack for the other files?

@bkeepers
Copy link
Contributor Author

Would it make sense to compile a single PR instead of PR per each file? Do you have a plan of attack for the other files?

Sure I can batch some together if you prefer. I don't really have a plan besides ls -Sr src/**/*.js to get the list of files remaining sorted smallest to largest and started on the first one I saw that didn't have other dependencies, but I can put together a little more of a plan and do a larger chunk.

@bkeepers
Copy link
Contributor Author

Also, I converted the signalk-tides plugin to TypeScript and ran into a lot of challenges that I plan to open PRs to address: bkeepers/signalk-tides#2

@tkurki tkurki added the chore label Mar 30, 2025
process.stderr.write = function (string) {
errWrite.apply(process.stderr, arguments)
storeOutput(string, true)
process.stderr.write = function (...args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't you doing function (...args: Parameters<typeof errWrite>) here? Similarly for outWrite.

Copy link
Contributor Author

@bkeepers bkeepers Apr 29, 2025

Choose a reason for hiding this comment

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

write is an overloaded function in typescript, and Parameters<typeof errWrite> only returns the first function signature, which results in a TypeScript error. See microsoft/TypeScript#32164 for more on this issue.

The ideal solution here is probably to do something like process.stderr = new Transform() { … } and piping that to the old stderr instead of just overwriting the write method, but I didn't want to make too invasive of changes here.

@bkeepers bkeepers force-pushed the logging-ts branch 3 times, most recently from 9957fb5 to 92d5f41 Compare April 29, 2025 19:26
@bkeepers
Copy link
Contributor Author

Would it make sense to compile a single PR instead of PR per each file? Do you have a plan of attack for the other files?

@tkurki I started working on batching a bunch of these together, but it started to feel like I am trying to boil the ocean. #1947 and #1948 were the result of some of this work. To avoid opening massive PRs like that, I think I'd prefer to keep these small and focused.

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

Successfully merging this pull request may close these issues.

3 participants