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

Allow lazy `message` #47

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@zaoqi
Copy link

commented May 11, 2019

No description provided.

@zaoqi

This comment has been minimized.

Copy link
Author

commented May 11, 2019

Sometimes calculating error messages is costly

@zaoqi

This comment has been minimized.

Copy link
Author

commented May 11, 2019

Sometimes calculating an error message when there is no error will make an error.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Thanks for the addition but this should be implemented in Node.js first. I did think about something like that before as well but just accepting any function would prevent other usages for this variable. I'll soon have a look at it to implement something like this or please feel free to open a PR in Node.js directly.

@zaoqi

This comment has been minimized.

Copy link
Author

commented May 11, 2019

@BridgeAR
What to implement in Node.JS?
ECMAScript's Error?
Open a PR in ECMAScript?

@zaoqi

This comment has been minimized.

Copy link
Author

commented May 11, 2019

@BridgeAR
What other usages for this variable?

https://tc39.github.io/ecma262/#sec-error-objects

@ljharb
Copy link

left a comment

I don't think this is a great API; the user can already pass { toString() { return 'lazy message'; } } if they like. if we wanted to allow them to pass a getter function for message (which i'm not convinced by either), then i'd think that should be a separate option rather than overloading message.

Show resolved Hide resolved assert.js Outdated
@goto-bus-stop

This comment has been minimized.

Copy link
Member

commented May 11, 2019

@zaoqi

What to implement in Node.JS?

This module is a port of Node.js's assert module for other engines. If you want to add a feature, it should be done in the nodejs/node repository first, and then it can be implemented here once it is released.

Update assert.js
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member

commented May 12, 2019

@zaoqi

What to implement in Node.JS?

See #47 (comment)

What other usages for this variable?

The assert module is very low level but it should still provide a good way to use this. I did consider overloading the message allowing the user to provide any type as input and to combine the inspected inputs value in combination with an automated error message in case of an error. That would not be possible in case we reserve functions for lazy evaluation.

@ljharb

I don't think this is a great API; the user can already pass { toString() { return 'lazy message'; } } if they like. if we wanted to allow them to pass a getter function for message (which i'm not convinced by either), then i'd think that should be a separate option rather than overloading message.

The toString possibility does not sound great to me either. Relying on a side effect is brittle and there is no support for that. We could theoretically use e.g., util.inspect to stringify the input. That way the message would be lost.

This problem is something that I ran into myself before as well and I think it's a good idea to implement something like this in general. The specific implementation aside.
What would be your alternative to overloading the message argument? The API does not provide much opportunities to do other things. It would be possible to use a single option for everything as i.e., assert.deepStrictEqual({ actual: foo, expected: bar, lazyMessage() { return 'foobar' } }) but otherwise we likely have to overload the message property.

I thought about adding a new assertion function which may be passed through as lazy message:

const assert = require('assert').strict

assert.deepEqual(foo, bar, assert.lazy(() => 'I am a lazy message'))`

That way it's at least expressive for someone else who reads the code and functions won't be special handled in general. What do you think about that solution?

@ljharb

This comment has been minimized.

Copy link

commented May 12, 2019

I don't even really understand the use case where the message is expensive to compute; but in that case I think a differently-named option would be ideal.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented May 12, 2019

@ljharb we had expensive messages in Node.js before:

We compared some entries of a complex structure and in case of an error we'd inspect the structure to know how it looked like.

That caused some tests to time out at some point due to inspecting multiple megabytes for lots of test cases instead of only inspecting the object in case of an error.

How would your ideal API look like in that case? I tried to outline the limited possibilities right now.

@ljharb

This comment has been minimized.

Copy link

commented May 12, 2019

We can bikeshed the name, but some straightforward option like “getMessage” - it'd throw if both that and message were provided, and if getMessage was not a function, and it’d invoke the function when it needed to display the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.