Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions benchmarks/warn.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ const err2 = createWarning('FastifyWarning', 'FST_ERROR_CODE_2', 'message')

new Suite()
.add('warn', function () {
err1.emit()
err2.emit()
err1()
err2()
})
.on('cycle', function (event) {
console.log(String(event.target))
Expand Down
2 changes: 1 addition & 1 deletion examples/example.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ const { createWarning } = require('..')

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

CUSTDEP001.emit()
CUSTDEP001()
68 changes: 31 additions & 37 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,41 +62,33 @@ function createWarning (name, code, message, { unlimited = false } = {}) {

code = code.toUpperCase()

return new WarningItem(name, code, message, unlimited)
}
let warning

/**
* Represents a warning item with details.
* @class
* @memberof processWarning
* @param {string} name - The name of the warning.
* @param {string} code - The code associated with the warning.
* @param {string} message - The warning message.
* @param {boolean} unlimited - If true, allows unlimited emissions of the warning.
*/
class WarningItem {
constructor (name, code, message, unlimited) {
this.name = name
this.code = code
this.message = message
this.unlimited = unlimited
this.emitted = false
}
const warningContainer = unlimited
? {
Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, especially considering the large object literal definitions (multiple lines), I would do:

let warningContainer = { [name]: function () {} }
if (unlimited === true) {
  warningContainer[name] = /* stuff */
} else {
  /* other stuff */
}

Ternaries are difficult to read and should be reserved for much simpler cases, in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be

  let warningContainer = {
    [name]: function (a, b, c) {
      if (warning.emitted === true && warning.unlimited !== true) {
        return
      }
      warning.emitted = true

      process.emitWarning(warning.format(a, b, c), warning.name, warning.code)
    }
  }
  if (unlimited) {
    warningContainer = {
      [name]: function (a, b, c) {
        warning.emitted = true

        process.emitWarning(warning.format(a, b, c), warning.name, warning.code)
      }
    }
  }

Do you want it this way?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is easier to read, yes. And with your suggestion, you don't have to redefine the whole object, just replace the method.

[name]: function (a, b, c) {
warning.emitted = true

/**
* Emits the warning.
* @param {*} [a] Possible message interpolation value.
* @param {*} [b] Possible message interpolation value.
* @param {*} [c] Possible message interpolation value.
*/
emit (a, b, c) {
if (this.emitted === true && this.unlimited === false) {
return
}
this.emitted = true
process.emitWarning(warning.format(a, b, c), warning.name, warning.code)
}

process.emitWarning(this.format(a, b, c), this.name, this.code)
}
}
: {
[name]: function (a, b, c) {
if (warning.emitted === true && warning.unlimited !== true) {
return
}
warning.emitted = true

process.emitWarning(warning.format(a, b, c), warning.name, warning.code)
}
}
warning = warningContainer[name]

warning.emitted = false
warning.message = message
warning.unlimited = unlimited
warning.code = code

/**
* Formats the warning message.
Expand All @@ -105,19 +97,21 @@ class WarningItem {
* @param {*} [c] Possible message interpolation value.
* @returns {string} The formatted warning message.
*/
format (a, b, c) {
warning.format = function (a, b, c) {
let formatted
if (a && b && c) {
formatted = format(this.message, a, b, c)
formatted = format(message, a, b, c)
} else if (a && b) {
formatted = format(this.message, a, b)
formatted = format(message, a, b)
} else if (a) {
formatted = format(this.message, a)
formatted = format(message, a)
} else {
formatted = this.message
formatted = message
}
return formatted
}

return warning
}

/**
Expand Down
4 changes: 2 additions & 2 deletions test/emit-interpolated-string.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ test('emit with interpolated string', t => {
}

const codeWarning = createWarning('FastifyDeprecation', 'CODE', 'Hello %s')
codeWarning.emit('world')
codeWarning.emit('world')
codeWarning('world')
codeWarning('world')

setImmediate(() => {
process.removeListener('warning', onWarning)
Expand Down
4 changes: 2 additions & 2 deletions test/emit-once-only.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ test('emit should emit a given code only once', t => {
}

const warn = createWarning('FastifyDeprecation', 'CODE', 'Hello world')
warn.emit()
warn.emit()
warn()
warn()
setImmediate(() => {
process.removeListener('warning', onWarning)
t.end()
Expand Down
2 changes: 1 addition & 1 deletion test/emit-set.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ test('emit should set the emitted state', t => {
warn.emitted = true
t.ok(warn.emitted)

warn.emit()
warn()
t.ok(warn.emitted)

setImmediate(() => {
Expand Down
2 changes: 1 addition & 1 deletion test/emit-unlimited.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ test('emit should emit a given code unlimited times', t => {

for (let i = 0; i < times; i++) {
expectedRun.push(i)
warn.emit()
warn()
}
setImmediate(() => {
process.removeListener('warning', onWarning)
Expand Down
4 changes: 2 additions & 2 deletions test/issue-88.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ test('Must not overwrite config', t => {
createWarning('FastifyWarning', 'CODE_2', 'Msg', { unlimited: true })

process.on('warning', onWarning)
a.emit('CODE_1')
a.emit('CODE_1')
a('CODE_1')
a('CODE_1')

setImmediate(() => {
process.removeListener('warning', onWarning)
Expand Down
2 changes: 1 addition & 1 deletion test/jest.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const { createWarning } = require('..')

test('works with jest', done => {
const code = createWarning('FastifyDeprecation', 'CODE', 'Hello %s')
code.emit('world')
code('world')

// we cannot actually listen to process warning event
// because jest messes with it (that's the point of this test)
Expand Down