fix: emitter api

[Eomm]

right now fastify relies on emitter as a map of string/bool.

This pr exposes that interface "officially"

Ref:

• fix(#5049): Remove duplicated calls to onReady fastify#5051
• fix(#5049): Remove duplicated calls to onReady fastify#5051 (comment)

Bench best of 3 run:

➜  process-warning git:(fix-map-api) ✗ node benchmarks/warn.js
warn x 104,268,444 ops/sec ±0.56% (97 runs sampled)
(node:21139) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:21139) [FST_ERROR_CODE_3] FastifyWarning: message

➜  process-warning git:(master) node benchmarks/warn.js
warn x 103,022,188 ops/sec ±0.31% (101 runs sampled)
(node:21559) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:21559) [FST_ERROR_CODE_3] FastifyWarning: message

[Uzlopak]

can we have that as separate variables? Why an object?

[Eomm]

Just for grouping the things I think - can drop it

[jsumners]

This comment should have been raised in #90. But the intention is to represent it as the enumeration that it is.

[Uzlopak]

whats this?

[Eomm]

crappy ideas

[Uzlopak]

Actually this looks like something where you could use bit flags to avoid alot of comparisons.

[Eomm]

Did I say that I ❤️ JS?

[Eomm]

Actually this looks like something where you could use bit flags to avoid alot of comparisons.

Not used to writing that black magic - do you have any source?
I agree that we need 2 bit only

[mcollina]

lgtm

[jsumners]

We might be able to make this easier to read and work with using:

'use strict'

class STATE extends Boolean {
  static UNLIMITED_INITIAL = 0
  static UNLIMITED_ONGOING = -1
  static LIMITED_INITIAL = 1
  static LIMITED_FINAL = 2

  #code = 0

  constructor(value) {
    super()
    this.#code = value ?? 0
  }

  get code() {
    return this.#code
  }

  set code(val) {
    this.#code = val
  }

  isEmitted() {
    return this.valueOf()
  }

  valueOf() {
    switch (this.#code) {
      case STATE.UNLIMITED_INITIAL: return false
      case STATE.UNLIMITED_ONGOING: return true 
      case STATE.LIMITED_INITIAL: return false
      case STATE.LIMITED_FINAL: return true
    }
  }
}

const state = new STATE(STATE.UNLIMITED_INITIAL)
console.log(state)
logState(state)

state.code = STATE.UNLIMITED_ONGOING
logState(state)

function logState(s) {
  if (s.isEmitted() === false) {
    console.log('not emitted')
  } else {
    console.log('emitted')
  }
}

Note that I added a .isEmitted() because Boolean(false) !== false.

[metcoder95]

Handed the work a little from @Eomm, I implemented your suggestion, can you take a look?

Although its kind of smaller codebase, its first time working on it so let me know if the changes are aligned with that you had in mind 👍

[Uzlopak]

I dont like it. Overcomplicated.

[Eomm]

I dont like it. Overcomplicated.

My scope is to keep only one Map and not change the emitter api, or I need to modify the Fastify's tests and release a semver major

Just for fun

[metcoder95]

I'm having quite of hard times understanding why this is identified by the coverage report as not covered, although I'm somehow explicitly testing it. Any suggestion is welcomed

[jsumners]

After taking a little more time to review this, I am not clear what we are trying to solve. I can't immediately find the usage in the Fastify project, but I believe what originally got broken somewhere along the line is this use case -- https://github.com/ldapjs/messages/blob/b73f34bc3769210af4b5805ef8d06c89d3e456ca/lib/ldap-message.test.js#L29-L56

Specifically:

t.teardown(async () => {
      process.removeListener('warning', handler)
      warning.emitted.set('LDAP_MESSAGE_DEP_001', false)
    })

If we merge the current suggestions in this thread, we will enter into an ambiguous state when resetting an emission in that manner. In my opinion, this cannot be fixed without a semver major that exposes an API for managing the emission state, and we should revert back to the simplistic true/false interface in the current version.

[metcoder95]

It seems we mostly use them for testing as you suggest, here's one example: https://github.com/fastify/fastify/blob/1891f243ab8666ef926218691135eb032008632a/test/default-route.test.js#L27

I'm not well versed on the initial design's goal and how this can be a breaking change; but I agree to keep it simple and go along the lines of something within minor/patch so fastify's CI can move forward.

Let me revert and go to the previous state to start from there.

[metcoder95]

I think I tried to go too far with the testing, thanks @jsumners for the suggestion. I think spoke too soon but do we want to keep with this or better go back to plain Boolean obj?

[jsumners]

I think there is at least the start of a good interface in this PR. It just isn't compatible with semver minor. I recognize that we did not document the export of the Map, but we also did not take any measures to make it private, either. So it is a public API, and one that is needed for applications that want to unit test their warnings (which should be any application using this library).

[metcoder95]

Then, wdyt if we point this to next and we create a fresh one following the same rationale, but with plain boolean?

[jsumners]

@metcoder95 the root of the issue was introduced in #78. It was further exacerbated in #89 and #90.

In short, we need an implementation with the "unlimited" support that uses the simple boolean approach. We can point this current PR to a next branch, but it'll need more updating to expose a better API (which I haven't given any thought to at this time).

[Eomm]

Hello, back again.

I agree, we have no better options
Releasing it as a minor would be cool only as a leet-code challenge.

I would proceed:

• Revert all the changes
  • revert 2.3.1 #93
• Release a patch so the Fastify CI will be fixed tho
• Restore the fix and drop the Map as public API and add a controlled interface
• Release a major
• Update Fastify accordingly