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

Optimisation for switch node #2258

Closed
wants to merge 6 commits into from
Closed

Optimisation for switch node #2258

wants to merge 6 commits into from

Conversation

namgk
Copy link
Contributor

@namgk namgk commented Aug 7, 2019

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Optimisation (improving load time for complex switch nodes from ~1.3 secs to ~0.6 secs )

Proposed changes

I usually have to work with complex switch nodes (>10 cases) and it is very slow whenever I open their configuration (~1-2 secs).

It seems it renders the typed fields eagerly, even without user interaction. It then chooses to hide and show certain DOM elements that were previously created based on the rule.

For complex switch node, this eager rendering results in tedious wait time even though the rules are very simple.

I'm proposing a simple lazy rendering of these fields to improve the load time of this node's configuration. Here we only render these fields on demand when the user choose to do so (e.g. select the rule).

With this approach, the performance could be significantly improved (tested with Chrome performance profiling, it improves load time for complex switch nodes from ~1.3 secs to ~0.6 secs ).

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the mailing list/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@coveralls
Copy link

coveralls commented Aug 7, 2019

Coverage Status

Coverage remained the same at 76.359% when pulling 0d680a5 on namgk:master into defa9a2 on node-red:master.

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this.

Only one small change I'd suggest - to avoid the expense of throwing/catching an error.

@knolleary knolleary added this to In progress - 1.0 in Development Aug 14, 2019
@knolleary
Copy link
Member

I've manually merged this into the dev branch - which is where new feature development lives - so I am going to close this PR.

In doing so I squashed a couple bugs I hit and also merged in some updates that had already been made in dev.

Thanks for this!

@knolleary knolleary closed this Aug 21, 2019
Development automation moved this from In progress - 1.0 to Done Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants