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

Remove unnecessary zoning on event handlers that interferes with testing #181

Merged
merged 7 commits into from
May 14, 2019

Conversation

greglittlefield-wf
Copy link
Collaborator

@greglittlefield-wf greglittlefield-wf commented Apr 30, 2019

Problem

  • The way the zone used for component lifecycle methods is determined is unpredictable: whatever zone the first registerComponent is called in is used, and all subsequent components will also use that zone as well.

    This means that if the first component is rendered in a test block, all other lifecycle methods will be run in that test's zone, resulting in output that:

    • makes errors in lifecycle methods look like they were caused by the first test
    • makes print statements in lifecycle methods look like they were caused by the first test

    Also, as a result, any event handlers added to a ReactElement within a component also had the same output issues.

  • The way the zone used for event handlers is determined gets in the way of testing: the zone in which the ReactElement with the event handler is used.

    This results in errors such as the following when wrapping expectAsync in event handlers or using expect in them, since they're executed in the root zone as opposed to the current test's zone.

    Bad state: Can't add or remove outstanding callbacks outside of a test body.

    This happens quite commonly when testing w_flux actions, or custom event-based prop callbacks.

Solution

  • Always use the root zone to run component lifecycle methods, since that's what it should be most of the time.
    I tried this out, and unfortunately it seems that many tests depend on this zoning, and actually run expect in other tests' zones... 😢 We'll have to revisit this.
  • Run event handlers in the zone they're dispatched in. This should always be the root zone outside of tests, and lets the test zone be used when simulating events.
    • Add tests

Testing

  • Verify unit tests pass
  • Pull into a large UI repo (e.g., WSD) and verify that all tests still pass
  • Verify in a single example that expectAsync no longer needs a zone-bound callback with these changes pulled in

@evanweible-wf
Copy link
Collaborator

QA +1

  • CI passes
  • Verified these changes allow us to remove usages of Zone.current.bindUnaryCallback() that were previously necessary for certain async callbacks
    • Tested in WSD, DPE, and w_project

@greglittlefield-wf

Copy link
Collaborator

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1

@greglittlefield-wf greglittlefield-wf merged commit 2149285 into master May 14, 2019
@greglittlefield-wf greglittlefield-wf deleted the zone-event-improvements branch November 21, 2019 21:26
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.

3 participants