-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: add types #155
feat: add types #155
Conversation
1e18e35
to
c81442b
Compare
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's the first time I convert so much code with JSDoc. Possibly uncovered one bug in the process, but I find all these types add a lot of noise.
src/lib/reducer.js
Outdated
isValueType(item.node.type) && item.node.value === 0); | ||
withoutZeroItem.unshift(firstZeroItem) | ||
isValueType(item.node) && item.node.value === 0); | ||
withoutZeroItem.unshift(/** @type Collectible*/(firstZeroItem)) |
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.
Potential bug? I don't see why firstZeroItem
could not be undefined
. Without this case TypeScript complains every time you get an element out of collected
.
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.
hm, but firstZeroItem
can be undefined
, we should add check firstNonZeroItem && firstNonZeroItem.preOperator === '-'
, but it is regarding how ts do checks, because logic is fine here
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've replaced the cast with checking if firstZeroItem
is truthy before unshifting it. It makes no sense no add an undefined
or null
item to the array anyway.
@@ -70,8 +88,11 @@ export default (node, property, options, result) => { | |||
? transformSelector(node[property], options, result, node) | |||
: transformValue(node[property], options, result, node); | |||
} catch (error) { | |||
result.warn(error.message, { node }); | |||
|
|||
if (error instanceof Error) { |
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.
Is there a way to avoid this?
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.
Using as
//** @type {Error} **/ (error)
c81442b
to
30ab635
Compare
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.
Looks good 👍
I've added |
No description provided.