feat: update adds improved jsdoc based message #173
Conversation
| * @param condition A boolean condition - if falsey will thrown an error. | ||
| * @param message The message provided to accompany the invariant. No inline default argument for message as the result is smaller. | ||
| * | ||
| * @example |
There was a problem hiding this comment.
haha yeah. Playful but I think it helps to make it really clear how to use the fn :)
src/tiny-invariant.ts
Outdated
| /** | ||
| * Throw an error if the condition fails. Strip out error messages for production. | ||
| * | ||
| * @param condition A boolean condition - if falsey will thrown an error. |
There was a problem hiding this comment.
Do we have to provide definitions for the @params? It feels like doubling up 🤔
There was a problem hiding this comment.
I wonder if this is enough as a jsdoc:
/** Throw an `Error` if the `condition` is falsey */There was a problem hiding this comment.
What do you think @DarkPurple141 ?
There was a problem hiding this comment.
Don't need them necessarily, really just there for completeness.
|
Hey @alexreardon I've updated this PR and also added type information into the jsdoc - here is how it appears in IDE. 
Hovering a 
The documentation also closely matches what's in the README.md file. This is compared to how it currently appears in IDE: 
While it is slightly more verbose - it doesn't add to the bundle and makes it usage more explicit. For those users not using typescript and inferring based on JSDoc this will also be beneficial. If any feedback or thoughts let me know. Happy to discuss on the PR! |
| @@ -4,6 +4,23 @@ | |||
|
|
|||
| const prefix: string = 'Invariant failed'; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
have also added the same jsdoc to the flow package
| export default function invariant( | ||
| condition: any, | ||
| // Can provide a string, or a function that returns a string for cases where |
There was a problem hiding this comment.
This is now covered directly in the jsdoc
src/tiny-invariant.ts
Outdated
| * @param {boolean} condition A boolean condition - if falsey will thrown an error. | ||
| * @param {(string|() => string)} message The message provided to accompany the invariant. Can provide a string, or a function that returns a string for cases where the message takes a fair amount of effort to compute. | ||
| * | ||
| * @returns {asserts condition is true} |
There was a problem hiding this comment.
asserts condition is truthy?
There was a problem hiding this comment.
{ } syntax is a hint for the TS server - so this is really
asserts condition (variable) is true <-- which is the ts expression
Can certainly add the explanation as an annotation tho next to it.
src/tiny-invariant.ts
Outdated
| * | ||
| * The `invariant` function takes a value, and if the value is falsy then the `invariant` function will throw. If the value is truthy, then the function will not throw. | ||
| * | ||
| * @param {boolean} condition A boolean condition - if falsey will thrown an error. |
There was a problem hiding this comment.
yeah this needs to be corrected - will address
|
@alexreardon I've updated let me know if all good 👍 |
|
Thanks for your effort on this @DarkPurple141. Rather than re-implementing the types in jsdoc, for now I've decided to keep the types in |
|
Hi Alex thanks - what you've merged is a improvement so I appreciate that it'll make it easier to parse in an IDE! I'm a bit disappointed that we couldn't merge this PR - I spent a decent amount of time nursing it. The types aren't redeclared, typescript annotations still win as per the attached screen grabs. This did however provide type hints where a typescript server is not present which is not uncommon. Maybe this context was lost in the thread of this PR? I feel like what's been merged is completely fine but borrows heavily from the context of this contribution. |
|
Hey Alex, I am sorry I disappointed you - it was not my intention to do anything wrong by you. I really appreciate your contribution. When I accept a contribution, I want to completely understand every line as at the end of the day - I'll be on the hook for it. I haven't looked into jsdoc type assertions, and I would want to check that what was in this PR behaved correctly in non-TS environments before merging. I didn't really have the energy to do that right now. I thought I would cut down the scope of this change (for now) to just be about making editor experiences better. Rather than asking you to modify this PR, I thought it would be easier for everybody for me to write and ship a small jsdoc comment based on the language in the existing documentation. Cheers |
Applies a jsdoc format instead to better explain usage.