Skip to content
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

Status #1

Closed
Steve-Mcl opened this issue Aug 13, 2020 · 15 comments · Fixed by #4
Closed

Status #1

Steve-Mcl opened this issue Aug 13, 2020 · 15 comments · Fixed by #4
Assignees
Labels
enhancement New feature or request implemented Work is done. Waiting for release.
Projects

Comments

@Steve-Mcl
Copy link

Hi @ollixx I think this is a fabulous addition to node-red - thank you for your contribution.

Would you consider adding "status" to the "use comp" node?

Mock up...
image

On the "Use Comp" node, permit the user to specify where status will be found in the msg
image

I would hope the status value would accept a string (for simple text status) or an object that conforms to the node.status api

NOTE: The Status setup might make more sense on the "return" node (but it would not be settable per "Use Comp" node) - either is fine by me :)

Thanks for the consideration


Other thoughts (constructive criticism) ...
As I have already said, I find this to be an excellent addition to node red - but I fear the naming might prevent users from discovering it. In programming, the functionality is analogous to a subroutine call. I wonder if node-red-contrib-subroutine would have been a better name (too late to change?)

IMHO, I think the nodes themselves might be better off called something like...

  • node-red-contrib-components --> node-red-contrib-subroutine
  • comp start --> Subroutine Define
  • comp return --> Subroutine Return
  • use comp --> Subroutine Call

(no offence intended - only my thoughts)

@ollixx ollixx added the enhancement New feature or request label Aug 17, 2020
@ollixx
Copy link
Owner

ollixx commented Aug 17, 2020

Hi Steve,
thanks for another very well prepared suggestion ;-)
Re the status, it totally makes sense to me. I will try to find some time for that in the near future.

I am not so convinced about the renaming, for some reasons:

  • subflows already exist. A new node being named subflow would be just as confusing (probably the most important con)
  • my idea really was to create self contained components, that can be used much more independently than i.e. the action flow nodes and have a defined interface - which the subflows don't.
  • if your concern is just about finding the node I could probably just add some more keywords to the package.
  • When you think of the components as a black box with a well defined interface, it doesn't feel right to call that a subroutine. I would rather make people think in terms of components, than subroutines, as for me that is an oldish expression from the GOTO times. But to be honest, since the component nodes (have to) pass though the complete message, it is not so self contained after all.

Let me elaborate some more about the last item:
One of the ideas not yet realized is, to somehow give the inner of the component a separate scope. That would mean, that the flow of the definition only sees the message parts defined in the interface. All other incoming message parts should than be hidden (maybe inside _comp). That of course brings up the question, how to handle the result of the component and how to merge it back into the original message. My idea here was to extend the interface for the results. That could be done by configuring the expected resulting parts and define the target in the message for them, using red.util.setMessageProperty.

What do you think?

@Steve-Mcl
Copy link
Author

Steve-Mcl commented Aug 17, 2020

Re the status, it totally makes sense to me. I will try to find some time for that in the near future.

Thank you for the consideration.
Did you consider where the setup of that status might be (in the return node? in the use comp node?)

One of the ideas not yet realized is, to somehow give the inner of the component a separate scope. That would mean, that the flow of the definition only sees the message parts defined in the interface. All other incoming message parts should than be hidden (maybe inside _comp). That of course brings up the question, how to handle the result of the component and how to merge it back into the original message

I regularly use properties that have been collected on route for debugging (at the end of a flow) & also often need other parts of msg for downstream nodes. Restricting what is in the msg would make this a non starter for me personally. Then there are instances where certain properties MUST be in the msg (like a http out or a split node sequence).
I personally think your node works perfectly as it is.

Regarding naming. Each to their own, I will end that part of the conversation with some final thoughts...

  • I (and others if you read the forum) had no clue about what it did until I tried it out. The name just isnt intuitive or relative to common programming constructs. (IMO)
  • As it stands, it is to all intents and purposes IS a subroutine - and a very good one at that.

Again, my options and no offence meant - it wont stop me using it (and promoting it on the forum where it suits a design).

@Steve-Mcl
Copy link
Author

Hi @ollixx is this project dead?

@ollixx
Copy link
Owner

ollixx commented Dec 16, 2020

Hey Steve,
no it is definitely not dead.
At the moment, I have no capacity to work on the issues and new ideas.
The project I am paid for needs all my attention. I hope, that I can do some stuff now, in the holidays.
Hope to talk to you soon...
Cheers
:oliver

@Steve-Mcl
Copy link
Author

Hi Oliver,

The project I am paid for needs all my attention

Life gets in the way :)

no worries, if you were abandoning it, I would would have forked it or asked to take it over.

Cheers.

@hanc2006
Copy link

Hi @ollixx (@Steve-Mcl) , I was inspired by your project and I started adding new features. What I have done so far:

COMMON

  • Fixed all display issue with editable lists
  • Events now are emitted directly to target (without going through all the components)
  • RUN COMPONENT can have 0 ore more outputs (if 0 no emit will be started)

RUN COMPONENT

  • Added parameters validation in the "run component" oneditsave
  • Added in "run component" the ability to manage property from a nested object using a dot path
  • In "run component" now the mandatory parameters are suffix with *
  • Added in "run component" a new button to find the component (inspired by link node)

COMPONENT IN

  • Added tab view (input parameters, output ports, linked component)
  • Added live change propagation. When something is changed in this node all changes are propagated to the linked nodes in real time (add/remove parameters, add/remove outputs port, change parameters attributes)

COMPONENT RETURN

  • Now this component is able to find "component in" nodes that are connected to it (in any part of the flow) and derives from the "component in" the output ports. This allows you to return the message in different outputs.

RUN COMPONENT

COMPONENT IN
tab in

tab out

tab links

COMPONENT RETURN

COMPONENT STATES
without outputs

with multiple outputs

@ollixx
Copy link
Owner

ollixx commented Feb 12, 2021

Hey @hanc2006,
this is just great!
I am very happy, that you did that. Please open a PR, so we can review and merge your new features.
I would invite @Steve-Mcl to join as a second reviewer.
Let me know, if you are ready to do so.

Phew... so I need to spend time again with the components.
I just had no time - even in corona times software projects need attention and so does my bank account ;-)

I am curious whether you wrote some tests to cover your stuff. The project definitely needs more tests.

Cheers
:oliver

@hanc2006
Copy link

Hey @ollixx ,
I can prepare one PR this weekend. I started editing this project for personal use, but then the changes increased and I thought I'd share them.

ehehe...I haven't thought about testing ... I'm sorry, but I can look further

@ollixx
Copy link
Owner

ollixx commented Feb 12, 2021

@hanc2006
please don't feel pushed regarding the testing.
We all know how important tests are and I make my money with testautomation for several years.
But still, I suppose you are not working on this node while being paid, so concentrate on a PR, so we can improve the nodes and share it with the community. I would assume, that you manually tested your work well enough to actually propose a PR.
We can always add more tests later :-)

@hanc2006
Copy link

Yes, yes I did some manual tests. What I can attach to the PR are some sample flows that covering different cases.

@hanc2006
Copy link

I forgot to mention the "new state property" proposed by @Steve-Mcl . I have implemented a simple version with the required specifications

COMPONENT RUN

@hanc2006
Copy link

Hey @ollixx,
sorry for the wait but I had a lot of work. I pushed the changes here fork before create a pull request. As I told you, I worked on this project for personal needs. I hope the changes are useful to others people.

As for the server components, there are few changes and adapting the tests should be simple. The most significant changes are on the client side, to manage the user interaction (delete, edit and add new components in the flow).

@ollixx
Copy link
Owner

ollixx commented Feb 26, 2021

thanks for your message @hanc2006.
It's a big change, so I will need some time to check it.

I don't understand, what you mean with "pushed before PR". You have to push to put your work into the repo. Now you could just open an PR in my project. But I will create a PR. so don't worry.

Thanks again for your effort. I hope I can release your stuff soon.

Greetingz!
:ollixx

@hanc2006
Copy link

I mean that I modified this repository using my own naming convention. I added new packages and introduced some code style rules that are personal and not related to this project. That's why I didn't create a PR right away.

@ollixx ollixx self-assigned this Jul 25, 2021
@ollixx ollixx linked a pull request Jul 25, 2021 that will close this issue
@ollixx ollixx added the implemented Work is done. Waiting for release. label Jul 25, 2021
@ollixx
Copy link
Owner

ollixx commented Jul 28, 2021

released with 0.1.9 - for the status feature.
The comments from @hanc2006 are yet to be considered. I implemented some of the points he mentioned here in this discussion:

  • fixed the layout inside the parameter lists.
  • implemented multiple output ports reflecting more than one return node. This is done quite differently from @hanc2006 's implementation. I hope it is more concise, as it does not need explicitly wiring the nodes, as it seems to be with his solution.

Stuff I like to take a look at (see Tickets):

@ollixx ollixx closed this as completed Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request implemented Work is done. Waiting for release.
Projects
Tickets
Awaiting triage
Development

Successfully merging a pull request may close this issue.

3 participants