timgit / pg-boss

Queueing jobs in Postgres from Node.js like a boss
MIT License
1.95k stars 153 forks source link

Types: Extend core events.EventEmitter #295

Closed jhermsmeier closed 2 years ago

jhermsmeier commented 2 years ago

This extends the PgBoss class declaration with the core EventEmitter, to enable usage of other EventEmitter methods, such as .removeListener(), and also updates the on() and off() return values accordingly.

It does bring in an implicit peer-dependency on @types/node for TypeScript users though, which I'm not quite sure about how to deal with best. Another option could be adding it as a (peer-)dependency in the package.json, but that would impact non-TypeScript users, and TypeScript users would most likely already have the core types present anyway.

coveralls commented 2 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 523b83d418a61a8d038b6275628c13439ce5d9b2 on jhermsmeier:ts-event-emitter into f7eb8225493e0ec14a646ecd5492df004a90fc76 on timgit:master.

timgit commented 2 years ago

It does bring in an implicit peer-dependency on @types/node for TypeScript users though, which I'm not quite sure about how to deal with best.

The risk is that the consumer wouldn't have already imported @types/node and does npm i --production (or NODE_ENV=production), so it would be missing? If that's the case, I also don't know what the normally acceptable solution would be.

Another option could be adding it as a (peer-)dependency in the package.json, but that would impact non-TypeScript users,

By impact, you mean it just makes the install size increase? I vote "this is fine" on that if so.

and TypeScript users would most likely already have the core types present anyway.

This I think addresses your first point about risk. What's the percentage of typescript apps not importing types?

jhermsmeier commented 2 years ago

By impact, you mean it just makes the install size increase? I vote "this is fine" on that if so.

Yes, exactly – the presence of an unnecessary dependency.

This I think addresses your first point about risk. What's the percentage of typescript apps not importing types?

I think it'd be the exception that there's a typescript app running on node that doesn't import the node core types.

The risk is that the consumer wouldn't have already imported @types/node and does npm i --production (or NODE_ENV=production), so it would be missing? If that's the case, I also don't know what the normally acceptable solution would be.

Yup, but then, with typescript it wouldn't even build, so the missing types would immediately become apparent, I think.

timgit commented 2 years ago

I think using devDependencies like you have in the PR is the best option for round 1. I've searched around github for other TS npm packages and it seems appropriate from what I've read. For example, the got package had quite a thread going earlier in the year about this, but it ended up being something very specific to their package.