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

fix(variable_hydration): limiting variable hydration to changing variables #18071

Merged
merged 9 commits into from
May 20, 2020

Conversation

asalem1
Copy link
Contributor

@asalem1 asalem1 commented May 12, 2020

Closes https://github.com/influxdata/idpe/issues/7030

Problem

This PR aims to address the issue of overly hydrating variables when a variable selection is made.

Solution

The solution to this is twofold:

  • Compare current selection to previous selection to ensure that selecting the same variable value will not trigger an action to hydrate the variables
  • Only pass in the variable that has been selected to hydrate that specific variable

Here's how these changes impact the network requests:

dehydrate-variables

UI Tweak

This PR also includes a minor UI tweak that may have been the result of the clockface pendulum update.

Previous:
Screen Shot 2020-05-12 at 14 26 10

Now:
Screen Shot 2020-05-12 at 15 05 58

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Rebased/mergeable

@asalem1 asalem1 requested review from drdelambre and a team May 12, 2020 22:19
Copy link
Contributor

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

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

its real hard to reason about the repercussions about this stuff. can we have a test that illustrates a dependent query being updated when the source query changes while the others stay the same?

@asalem1 asalem1 force-pushed the fix/variable-hydration branch 2 times, most recently from 928f4c6 to c9f1eff Compare May 18, 2020 22:46
@asalem1 asalem1 requested a review from drdelambre May 19, 2020 17:12
// use an ID array to reduce the chance of reference errors
const varIDs = variables.map(v => v.id)
// create an array of IDs to reference later
const graphIDs = []
Copy link
Contributor

Choose a reason for hiding this comment

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

note for self: this was because Set.prototype.has compares by reference, which we dont guarantee in an immutable kind of world

// use an ID array to reduce the chance of reference errors
const varIDs = variables.map(v => v.id)
// create an array of IDs to reference later
const graphIDs = []
Copy link
Contributor

Choose a reason for hiding this comment

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

nit for Ari: this should be an object for a faster lookup on line 148, but i can't imagine the search space being very large, so it's not a blocker

@asalem1 asalem1 force-pushed the fix/variable-hydration branch from 80d9205 to ba108df Compare May 19, 2020 21:00
@asalem1 asalem1 requested a review from drdelambre May 19, 2020 21:01
Copy link
Contributor

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

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

functionality was written quickly, and i'm pretty sure we've narrowed this down to a bug, wrote a test, and the best we can do now is launch it to see what happens

@asalem1 asalem1 merged commit 0c44328 into master May 20, 2020
@asalem1 asalem1 deleted the fix/variable-hydration branch May 20, 2020 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants