fix: emitter api by Eomm · Pull Request #91 · fastify/process-warning

Eomm · 2023-11-20T07:49:41Z

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

This pr exposes that interface "officially"

Ref:

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 · 2023-11-20T10:12:03Z

index.js

+const STATES = {
+  UNLIMITED_INITIAL: 0,
+  UNLIMITED_ONGOING: -1,
+  LIMITED_INITIAL: 1,
+  LIMITED_FINAL: 2
+}


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

Just for grouping the things I think - can drop it

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

Uzlopak · 2023-11-20T10:12:23Z

index.js

+  // const emitInterface = new Proxy(emitted, {
+  //   get (target, prop, receiver) {
+  //     const val = Reflect.get(emitted, prop, emitted);
+
+  //     if (prop === 'get') {
+  //       return function (code) {
+  //         const state = emitted.get(code)
+  //         return state === STATES.UNLIMITED_ONGOING || state === STATES.LIMITED_FINAL
+  //       }
+  //     }
+  //     return Reflect.get(emitted, prop, emitted);
+  //   }
+  // })
+


whats this?

crappy ideas

index.js

Uzlopak · 2023-11-20T15:47:59Z

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

Eomm · 2023-11-20T17:35:43Z

index.js

 module.exports.processWarning = processWarning
+
+function newBooleanState (code) {
+  return Object.create(Boolean, { stateCode: { value: code } })


Did I say that I ❤️ JS?

Eomm · 2023-11-20T17:40:19Z

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 · 2023-11-20T19:59:06Z

index.js

+const STATES = {
+  UNLIMITED_INITIAL: newBooleanState(0),
+  UNLIMITED_ONGOING: newBooleanState(-1),
+  LIMITED_INITIAL: newBooleanState(1),
+  LIMITED_FINAL: newBooleanState(2)
+}


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.

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 · 2023-11-20T20:06:42Z

I dont like it. Overcomplicated.

Eomm · 2023-11-20T20:20:27Z

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 · 2023-11-24T10:27:48Z

index.js

+  /* istanbul ignore next */
+  isEmitted () {
+    return this.valueOf()
+  }
+
+  valueOf () {
+    /* istanbul ignore next */


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 · 2023-11-24T13:48:12Z

index.js

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.

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 · 2023-11-26T11:49:44Z

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 · 2023-11-26T12:47:11Z

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 · 2023-11-27T14:39:57Z

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

jsumners · 2023-11-27T15:25:04Z

@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 · 2023-12-02T15:06:56Z

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

Eomm added 2 commits November 20, 2023 08:48

fix: emitter api

e5a1eec

lint

9ddd040

Uzlopak reviewed Nov 20, 2023

View reviewed changes

jsumners reviewed Nov 20, 2023

View reviewed changes

index.js Outdated Show resolved Hide resolved

jsumners reviewed Nov 20, 2023

View reviewed changes

index.js Outdated Show resolved Hide resolved

Eomm marked this pull request as draft November 20, 2023 17:11

fix: better

9e4e90b

Eomm commented Nov 20, 2023

View reviewed changes

Eomm marked this pull request as ready for review November 20, 2023 17:35

lintttttttttttt

3925f96

mcollina approved these changes Nov 20, 2023

View reviewed changes

jsumners reviewed Nov 20, 2023

View reviewed changes

metcoder95 mentioned this pull request Nov 21, 2023

docs(#5142): aligned errors and warnings documentation fastify/fastify#5162

Merged

4 tasks

metcoder95 added 2 commits November 24, 2023 10:31

refactor: address review changes

b340ed0

test: adjust coverage

3ab3ab8

metcoder95 reviewed Nov 24, 2023

View reviewed changes

jsumners mentioned this pull request Nov 24, 2023

Add tests for bool state #92

Merged

Add bool-state testing

80d05ed

jsumners requested changes Nov 24, 2023

View reviewed changes

Merge pull request #92 from fastify/fix-map-api-jsumners

f5b30df

Eomm mentioned this pull request Dec 2, 2023

revert 2.3.1 #93

Merged

Eomm closed this Dec 2, 2023

Uh oh!

Conversation

Eomm commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uzlopak commented Nov 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eomm commented Nov 20, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metcoder95 Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Nov 20, 2023

Uh oh!

Eomm commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metcoder95 Nov 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metcoder95 commented Nov 26, 2023

Uh oh!

jsumners commented Nov 26, 2023

Uh oh!

metcoder95 commented Nov 27, 2023

Uh oh!

jsumners commented Nov 27, 2023

Uh oh!

Eomm commented Dec 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Eomm commented Nov 20, 2023 •

edited

Loading

metcoder95 Nov 24, 2023 •

edited

Loading

Eomm commented Nov 20, 2023 •

edited

Loading

metcoder95 Nov 26, 2023 •

edited

Loading