-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
index.js
Outdated
warning.emitted = false; | ||
warning.message = message; | ||
warning.unlimited = unlimited; | ||
Object.defineProperty(warning, 'name', { value: name }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one slows it down, because it has to lookup the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because a function has a non writable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we accept a name
parameter as input, that can be read by the user.
@Uzlopak What if we rename it to group
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think name is important to detect DeprecationWarnings so that node would ignore them.
The function name is usually set by doing function NAME () {}
or const NAME = () => {}
, where NAME
is the desired Function Name. But I dont know how I could do it at runtime. Maybe by doing abusing new Function
, which is basically a eval
call. Also some runtimes disable, i think cloudflare, disallow new Function
because it is under the hood an eval.
It could be also done by doing:
const NAME = 'DeprecationWarning'
const container = {
[NAME]: () => {}
}
console.log(container[NAME].name) // 'DeprecationWarning'
But something like
function createNamedFunction (name, fn = () => {}) {
const container = {
[name]: fn
}
return container[name]
}
const bla = createNamedFunction('DeprecationWarning', () => {})
console.log(bla.name) // '' <-- empty
Removed the bottleneck |
const warningContainer = unlimited | ||
? { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the docs? Code looks good.
@mcollina |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this
Can you update the PR title and description? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warn x 156,722,644 ops/sec ±0.16% (102 runs sampled)
(node:38030) [FST_ERROR_CODE_1] FastifyWarning: message
(Use `node --trace-warnings ...` to show where the warning was created)
(node:38030) [FST_ERROR_CODE_2] FastifyWarning: message
* feat!: new api * test: update code * fix: issue * update bench * docs * ts * jsdoc * fix typings * Update README.md Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com> * Update README.md Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com> * new api * new api ts * fix docs * Implement function scoped Warning (#97) * Apply suggestions from code review Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com> Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com> * fix feedback * fix docs --------- Co-authored-by: uzlopak <aras.abbasi@googlemail.com> Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com>
Just for demonstration purposes
Checklist
npm run test
andnpm run benchmark
and the Code of conduct