Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement function scoped Warning #97

Merged
merged 7 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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 @@ -16,8 +16,8 @@ const err2 = createWarning({

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 @@ -8,4 +8,4 @@ const CUSTDEP001 = createWarning({
message: 'This is a deprecation warning'
})

CUSTDEP001.emit()
CUSTDEP001()
79 changes: 41 additions & 38 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ const { format } = require('node:util')

/**
* Represents a warning item with details.
* @typedef {Object} WarningItem
* @typedef {Function} WarningItem
* @param {*} [a] Possible message interpolation value.
* @param {*} [b] Possible message interpolation value.
* @param {*} [c] Possible message interpolation value.
* @property {string} name - The name of the warning.
* @property {string} code - The code associated with the warning.
* @property {string} message - The warning message.
Expand Down Expand Up @@ -44,6 +47,14 @@ function createDeprecation (params) {
return createWarning({ ...params, name: 'DeprecationWarning' })
}

/**
* Emits the warning.
* @typedef WarningEmitter
* @param {*} [a] Possible message interpolation value.
* @param {*} [b] Possible message interpolation value.
* @param {*} [c] Possible message interpolation value.
*/

/**
* Creates a warning item.
* @function
Expand All @@ -60,41 +71,31 @@ function createWarning ({ name, code, message, unlimited = false } = {}) {

code = code.toUpperCase()

return new WarningItem(name, code, message, unlimited)
}
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

/**
* 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
}
process.emitWarning(warning.format(a, b, c), warning.name, warning.code)
}

/**
* 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
}
}
: {
[name]: function (a, b, c) {
if (warning.emitted === true && warning.unlimited !== true) {
return
}
warning.emitted = true

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

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

/**
* Formats the warning message.
Expand All @@ -103,19 +104,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 @@ -19,8 +19,8 @@ test('emit with interpolated string', t => {
code: 'CODE',
message: '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 @@ -19,8 +19,8 @@ test('emit should emit a given code only once', t => {
code: 'CODE',
message: '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 @@ -20,7 +20,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 @@ -28,7 +28,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 @@ -23,8 +23,8 @@ test('Must not overwrite config', t => {
})

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 @@ -9,7 +9,7 @@ test('works with jest', done => {
code: 'CODE',
message: 'Hello world'
})
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
4 changes: 2 additions & 2 deletions types/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
declare namespace processWarning {
export type WarningItem = {
export interface WarningItem {
(a?: any, b?: any, c?: any): void;
name: string;
code: string;
message: string;
emitted: boolean;
unlimited: boolean;
emit(a?: any, b?: any, c?: any): void;
format(a?: any, b?: any, c?: any): string;
}

Expand Down
12 changes: 6 additions & 6 deletions types/index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ expectType<string>(WarnInstance.name)
expectType<boolean>(WarnInstance.emitted)
expectType<boolean>(WarnInstance.unlimited)

expectType<void>(WarnInstance.emit())
expectType<void>(WarnInstance.emit('foo'))
expectType<void>(WarnInstance.emit('foo', 'bar'))
expectType<void>(WarnInstance())
expectType<void>(WarnInstance('foo'))
expectType<void>(WarnInstance('foo', 'bar'))

const buildWarnUnlimited = createWarning({
name: 'FastifyWarning',
Expand All @@ -31,6 +31,6 @@ const DeprecationInstance = createDeprecation({
})
expectType<string>(DeprecationInstance.code)

DeprecationInstance.emit()
DeprecationInstance.emit('foo')
DeprecationInstance.emit('foo', 'bar')
DeprecationInstance()
DeprecationInstance('foo')
DeprecationInstance('foo', 'bar')