Refactoring (v3)

[Eomm]

fixes #95, #88

New Bench:

➜  process-warning git:(major) ✗ node benchmarks/warn.js
warn x 156,880,997 ops/sec ±0.26% (100 runs sampled)
(node:5524) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:5524) [FST_ERROR_CODE_2] FastifyWarning: message

Master branch:

➜  process-warning git:(master) ✗ node benchmarks/warn.js
warn x 62,825,315 ops/sec ±0.21% (100 runs sampled)
(node:5718) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:5718) [FST_ERROR_CODE_3] FastifyWarning: message

TODO

• docs
• jsdocs
• typescript

[Uzlopak]

@Eomm
How about not using .emit() method at all but directly return the function?

const { createWarning } = require('..')

const CUSTDEP001 = createWarning('DeprecationWarning', 'CUSTDEP001', 'This is a deprecation warning')

CUSTDEP001()

?

[Eomm]

And how do you check if it has been emitted?

[Uzlopak]

CUSTDEP001.emitted ?

[mcollina]

I don't think switching to a class is needed, using a closure or function would be sufficient and avoid less changes.

[Eomm]

I don't think switching to a class is needed, using a closure or function would be sufficient and avoid less changes.

Did a quick test without class, and the performance was worse.
Here is the code and the results:

• https://github.com/fastify/process-warning/blob/no-class/index.js

NO CLASS:
➜  process-warning git:(major) ✗ node benchmarks/warn.js
warn x 65,740,447 ops/sec ±0.13% (101 runs sampled)
(node:7193) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:7193) [FST_ERROR_CODE_2] FastifyWarning: message

class:
➜  process-warning git:(major) ✗ node benchmarks/warn.js
warn x 152,302,444 ops/sec ±1.18% (97 runs sampled)
(node:8600) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:8600) [FST_ERROR_CODE_2] FastifyWarning: message

CUSTDEP001.emitted ?

TBH I don't like too much props attached to a function. I can't see any advantages, plus I expect the typings are more hard to write

[jsumners]

TBH I don't like too much props attached to a function.

I find this odd when considering how JavaScript works. A function is an object like any other.

[Eomm]

TBH I don't like too much props attached to a function.

I find this odd when considering how JavaScript works. A function is an object like any other.

Yeah, I like it too:

in this case, where we don't have constraints, and we are defining a new API, so I think that having a more expressive warninstance.emit() function is more evident than warn('foo', 'var') because it tells me what is happening and anyone that use it in whatever repository, can search for emit() to find where a warning is emitted.

[mcollina]

lgtm

[Eomm]

Need help for TS 😅

[Uzlopak]

Ok, will do now the TS part

[Uzlopak]

@Eomm

Fixed typings

[Eomm]

Good to go or strong against this API?

[Uzlopak]

LGTM

[Uzlopak]

Imho object param for constructor/factry is preferable

I am inconclusive regarding the payload for instanciating. Could mean that we need to create unnecessary intermediary arrays.

[mcollina]

I would generically prefer a functional pattern instead of a classical one:

const emit = createWarning(..)

emit(...)

if (emit.emitted) { ... }

This is just personal preference.

[Uzlopak]

Me too, but There is a performance bottleneck to implement it as functional, because the function name has to be set via Object.defineProperty. So we get from 220 mio ops/s and get 86 mio ops/s on my machine.

[jsumners]

I would generically prefer a functional pattern instead of a classical one:

const emit = createWarning(..)

emit(...)

if (emit.emitted) { ... }

This is just personal preference.

This is what the PR implements, correct?

Me too, but There is a performance bottleneck to implement it as functional, because the function name has to be set via Object.defineProperty. So we get from 220 mio ops/s and get 86 mio ops/s on my machine.

I don't follow. What has to be defined?

Personally, I don't think there is any need to sacrifice good api/developer experience in the name of "performance" here. These entities are going to be created at application initialization more often than not, and once created will perform as well as any other event emitter.

[Uzlopak]

See here:
#97

[Eomm]

This is what the PR implements, correct?

A bit different, there is the emit method to call:

const warning = createWarning('FastifyWarning', 'FSTWRN001', 'Hello %s', { unlimited: true })
warning.emit('world')

if (warning.emitted) { ... }

Fixing the input object

[mcollina]

lgtm

[Uzlopak]

@Eomm
@mcollina
Should I invest more time on #97
I guess i coule fix the perf issue. But if you say this PR solves the issue sufficiently, then I am ok with it.

[Uzlopak]

I could remove the bottleneck in #97

[Uzlopak]

I think the last open discussion point is if unlimited should be true by default or not.

I have no strong opinion on this.

[Uzlopak]

@jsumners
Pardon me.

I closed some of your remarks regarding the FST_ prefix in the Readme.md, because I thought they were addressed already as they were marked as Outdated.
Now i checked again and saw that some changes were not applied.

@Eomm
Can you please take care of them?

[jsumners]

I'll do a pass on the docs and examples after this is merged. None of my feedback has been blocking.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.