Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Notify on keydown #9
base: master
Are you sure you want to change the base?
Notify on keydown #9
Changes from 13 commits
3a57ff7
21d6254
0182e76
b433f05
0277a29
f735891
b8a5bdf
9c14e4a
5e8d465
cdf12ec
64e0859
ad69f26
f48cca2
d81ee4a
b4c740b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Won't this run
done()
a second time if theend()
succeeds and the firstdone()
throws an exception?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.
No, done shouldn't throw an exception since it's a callback provided by nodered. The catch is in case the end() throws an exception to ensure we still force the cleanup and call done().
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 shouldn't throw, but you don't control nodered's code. This would be safer and simpler to write as
This way an exception from
done()
won't cause it to repeat (or to calldestroy()
), and you're not repeating the callback either.Also, question: What happens if
destroy()
throws an exception? Neither version of the code will calldone()
in that case. Maybe it should? If it should, then my version would use.finally(() => done())
instead (or just.finally(done)
asfinally
already passes no arguments).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.
Ah, forgot finally was available with promises. Thanks, I'll update.
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.
Pre-existing issue, but this is missing Button 2 (
a=3
), and it's also missing the buttons for the PJ2-4B model (8
/9
/10
/11
), which is rather a problem as that's the model I have 😅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 should likely test
|| (value === 4 && node.sendObj)
. I know the only documented actions for pico remotes are 3 and 4 but that's no reason to make assumptions.