fix(variable-hydration): fixed variable overhydration issue #18346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asalem1
force-pushed
the
fix/variable-hydration-issue
branch
from
8d087aa
to
907f974
Compare
drdelambre
reviewed
Jun 4, 2020
| @@ -433,7 +434,10 @@ export const selectValue = (variableID: string, selected: string) => async ( | |||
|
|
|||
| await dispatch(selectValueInState(contextID, variableID, selected)) | |||
| // only hydrate the changedVariable | |||
| dispatch(hydrateVariables(true)) | |||
| // dispatch(hydrateChangedVariable(variableID)) | |||
| if (isFlagEnabled('hydratevars')) { | |||
There was a problem hiding this comment.
this is the most badass thing i've ever seen
There was a problem hiding this comment.
pushing the limits of feature flagging:
drdelambre
reviewed
Jun 4, 2020
| expect(actual.length).toEqual(1) | ||
| const [subgraph] = actual | ||
| // expect the subgraph to return the passed in variable | ||
| expect(subgraph.variable).toEqual(timeRangeStartVariable) | ||
| // expect the parent to be returned with the returning variable | ||
| expect(subgraph.parents[0].variable).toEqual(associatedVariable) | ||
| }) | ||
| test('should filter out inputs that have already been loaded based on a previous associated variable', async () => { |
There was a problem hiding this comment.
omg this documentation make my week
drdelambre
approved these changes
Jun 4, 2020
There was a problem hiding this comment.
thanks for all the tests! it makes accepting changes within this area of the code easier. even though there's some feature flags in there to control the peripheral, it seems like the internals aren't entirely disconnected when toggling the flag. that keeps this PR risky. given the history around this feature, it might be good to schedule some time with russ to run through this branch locally (iirc he has a comprehensive variable dataset for OSS testing) before merging to master.
asalem1
changed the title
|
We tested this locally a few hours ago and this is what we discovered:
This PR is a step in the right direction and doesn't appear to have broken anything, so i'm all for merging it! |
… have 1 reference to a node no matter how deeply nested Still need to update the filterUnusedVars function to load variables that might be called within variables Also need to determine whether a child node in a graph can have multiple parents
Need to figure out whether some of the tests are still necessary. Need to find out whether a child can have multiple parents Need to find out whether a parent with multiple children should hydrate all the children Need to work on the filterUnusedVars
…ith implementation
…m unsure where to go. Need to take a break from variables
…e constraints around parent/child relationships Added/updated tests to ensure feature stability Need to resolve filterUnusedVariables issues
…aph implementation so I removed the comment also add the featureflag back to the feature
… based on featureFlag This is a temporary workaround until the featureflag can be removed so that the tests pass
asalem1
force-pushed
the
fix/variable-hydration-issue
branch
from
66ec9ac
to
332bb51
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #18192
Problem
This PR aims to address the issue of overly hydrating variables when a variable selection is made.
Solution
The solution is as follows:
This solution will help reduce the amount of unnecessary query requests made from the UI post order to hydrate variables. However, since it involves a lot of moving parts that currently work, I've set it behind a feature flag in order to make sure that variables are still going to work correctly