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

Don't render parts for self-originated changes #52

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

quigleyj-mavenomics
Copy link
Member

@quigleyj-mavenomics quigleyj-mavenomics commented Nov 7, 2019

This PR includes an experimental change to evaluate the impact of #36, which proposes not rendering parts that update their own options.

First impressions are positive so far- the SlickGrid in particular seems more stable and performant. It seems like the wrapper was written for this execution model, which makes sense considering it's history in the enterprise platform.

We'll need to test a variety of UDPs and dashboards to ensure that this doesn't cause unworkable breakage, and going through the process of fixing UDPs that are broken by this change should tell us more about the real-world, practical impact of this.

Post-experiment To Do

After that's done, there's still some followup before this can be merged onto mainline:

  • Review Parts and UDPs
    • Various UDPs, and possibly a built-in part or three, rely on this behavior.
  • Review SlickGrid
    • This needs careful study:
      • the grid shouldn't be relying on this...
      • ...but might be as a matter of course.
    • Review:
      • Broken Column formatting
        • Conditional formatting
        • Heatmaps/progress bars
        • Sparklines
        • Dashboard links
      • Change highlighting
      • Tick Performance
        • No major regressions in Fund Summary tick-to-render time
        • (Appears to have actually improved, which wasn't expected)
  • Review demos for correctness:
    • PnL6*
    • Fund Summary*
    • (requires Jupyter) Rocket Motors
    • Table Editor*
    • *slickgrid cannot update column formatting appropriately
  • Review docs
    • The existing inline part docs are still mostly correct
      • just need a few tweaks here and there
    • No public docs on part writing, so no updates needed there
  • Update PartManager unit tests
    • Remove hacks I inserted
      • I disabled a number of tests to force builds to pass, so that we can test it
    • Remove places where we test for old behavior
    • Ensure that we test this new behavior

Will fix #36.

Known breakages:

  • SlickGrid
    • A lot of Grid features break in some way; esp. column formatting options (most of which don't apply until the next full render)
    • SlickGrid will need some work on these items, but we can find ways of working around them for now.

@quigleyj-mavenomics quigleyj-mavenomics added enhancement New feature or request parts Issues with Parts, Bindings, or the PartManager TBD Issue that isn't yet fleshed out enough to work on, but should still be percolating labels Nov 7, 2019
@quigleyj-mavenomics quigleyj-mavenomics force-pushed the feature/no-rerender-on-self-changes branch from 15e2141 to 53df560 Compare November 12, 2019 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request parts Issues with Parts, Bindings, or the PartManager TBD Issue that isn't yet fleshed out enough to work on, but should still be percolating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parts re-render on self-initiated changes
1 participant