bump eslint-plugin-jsdoc #5269
bump eslint-plugin-jsdoc #5269
Conversation
|
I remember some problem with using |
Oh my. Indeed. Though it appears to do the same for |
Right, |
There was a problem hiding this comment.
I need to pay closer attention to all the changes, and do some experiments in the TS Playground.
While Object -> object should be safe for any @typedef {object} or @param {object} used as the base of other @param, I'm not certain when used as return values or in TS style functions types it'd be safe.
I agree it would be great to fix all our Object usages, as they're all most likely wrong, but I'd rather get it right than change it into another wrong. Maybe we can file an issue and pick this up after MN-1, or there was a motivation for this change now besides declared support for Node 18?
| * @param {(baseRef: string) => Object} fetch Function to retrieve an | ||
| * @param {(baseRef: string) => object} fetch Function to retrieve an | ||
| * object's raw state from the store by its baseRef | ||
| * @param {(baseRef: string, rawState: Object) => void} store Function to | ||
| * @param {(baseRef: string, rawState: object) => void} store Function to |
There was a problem hiding this comment.
Here is potentially a place where it'd revert back to any
There was a problem hiding this comment.
It was already any. That's my point in the statement,
Though it appears to do the same for Object so not relevant to this change?
Object
object
| @@ -140,7 +140,7 @@ export function makeCache(size, fetch, store) { | |||
| * of virtual references. | |||
| * @param {() => number} allocateExportID Function to allocate the next object | |||
| * export ID for the enclosing vat. | |||
| * @param { (val: Object) => string} getSlotForVal A function that returns the | |||
| * @param {(val: object) => string} getSlotForVal A function that returns the | |||
There was a problem hiding this comment.
I believe it's the same here, val would now become an any type.
There was a problem hiding this comment.
now become an
anytype
Again, already was.
What are your concerns? From what I see, all the
Yes, motivation was to update the dep to support Node 18. I thought fixing the warnings would be good to include now and safe. If you don't think so I can remove that and suppress the warnings instead. But based on…
Will you Approve so I can merge? |
There was a problem hiding this comment.
What are your concerns? From what I see, all the
Objectareanybefore and after.
I had missed that all usages of Object also resulted in any, making it strictly equivalent to object in JSDoc.
As long as we've confirmed that there is no side-effect on tsc checks from using object, I'm ok with this change
Will you Approve so I can merge?
Given that Object and object are as bad as each other in all usages, approved.
turadg
added
the
automerge:no-update
Description
The version we have would error when doing
yarn installunder Node 18. (not in its list of supported engines)The latest version changed gajus/eslint-plugin-jsdoc#855 to align more with TypeScript. This fixes those warnings too (mostly with
yarn lint-fixand then manually a few stragglers)Security Considerations
--
Documentation Considerations
--
Testing Considerations
--