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
react nodes #118
react nodes #118
Changes from 1 commit
080da42
148c2d7
9f1dd96
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.
Type of
bounced
should be the same as the type ofsync
.You can formulate it like 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.
not sure need more details on what is meant?
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.
reviewing the command runner node. It is using the same types as have used although the error output type is now set to {"error": string} as suggested.
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.
We should point out in the descriptions that
dependencies
andprojectPath
are not protected from injection attack. (Or validate in code that they are actually path and package names)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.
Need to makes sure types reflect those of the internal code node. (see comments below)
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.
Most of the issues in the dependency installer repeat here, re. port types, serialization of
stdout
/stderr
, etc.Beyond that I feel the
success
output is redundant, as we either send something onstdout
orerror
. If I need to turn that into a boolean, I can use aboolean/Reverse condition
node.One other thing I'd like to ask is to wrap this node into a structure node with an identical interface. As a general rule, we give preference to implementing nodes using existing nodes rather than JS, so this would keep the functionality of this node future-proof re. refactoring.