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

column tree distributes width more safely #530

Merged
merged 1 commit into from
May 18, 2018

Conversation

addepar-andy
Copy link
Contributor

There is a bug where the width distributor will get into an infinite loop. The loop would check for delta being zero, but if delta started as a decimal, then the check would never pass. This does 3 things:

  1. adds a loop guard so that the UI / tests don't freeze if something crazy happens to the loop.
  2. rounds delta so the target is always an integer
  3. makes delta chunks always integral and sum to the delta

Reviewers: @Addepar/web-core

@addepar-andy
Copy link
Contributor Author

i can write some unit tests for the new function i wrote, but i'm not really sure how to actually trigger this bug, so i'm not sure what kind of component test to write.

@addepar-andy
Copy link
Contributor Author

i have also verified that this does fix the frozen tests on my iverson branch

Copy link
Contributor

@pzuraq pzuraq left a comment

Choose a reason for hiding this comment

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

I like the divideRounded function a lot, it's what I was trying to do but better!

I think we should wrap the loopCount logic in a DEBUG flag so that it gets stripped out of production builds though. You can import the flag, and use it in if statements and they'll be stripped:

import { assert } from '@ember/debug';
import { DEBUG } from '@glimmer/env';

let loopCount;

if (DEBUG) {
  loopCount = 0;
}

// ...

if (DEBUG) {
  loopCount++;
  assert('loop count exceeded guard while distributing width', loopCount < LOOP_COUNT_GUARD);
}

@addepar-andy
Copy link
Contributor Author

i think we should keep the flag in. if something goes wrong on prod and guard is there, users will see an error. without the guard, the entire tab will freeze.

@pzuraq
Copy link
Contributor

pzuraq commented May 18, 2018

We can keep it in for now, but I’d like to circle back and add a few more assertions that give us guarantees before we release 1.0.0. It shouldn’t be possible to get in that state.

@addepar-andy addepar-andy merged commit cdbd543 into master May 18, 2018
@addepar-andy addepar-andy deleted the andy/col-width-distribution branch May 18, 2018 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants