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(circus): make sure to install globals before emitting setup event #10598

Merged
merged 8 commits into from
Oct 6, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 6, 2020

Summary

Fix for #10484 (comment)

In addition to restoring time when globals are added, I've also added a property to the setup event that comes with the globals. That way consumers don't have to care about this.globals.*, which would also be unset if the end user use injectGlobals: false.

/cc @noomorph

Test plan

Verified locally that globals are set during setup

await dispatch({
name: 'setup',
parentProcess,
runtimeGlobals,
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we document this somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

we probably should, but the events as of now are not documented so I'd rather do that separately at some point

@SimenB SimenB changed the title fix(circus): make sure to install globals before emitting init event fix(circus): make sure to install globals before emitting setup event Oct 6, 2020
@noomorph
Copy link
Contributor

noomorph commented Oct 6, 2020

Looks good, I'll give it a try on the Detox suite.

@noomorph
Copy link
Contributor

noomorph commented Oct 6, 2020

Btw, the idea of emitting runtimeGlobals looks fantastic and fun to use. 👍 kudos!

@@ -9,12 +9,11 @@ import micromatch = require('micromatch');
import type {Config} from '@jest/types';
import replacePathSepForGlob from './replacePathSepForGlob';

type Matcher = (str: Config.Path) => boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

totally unrelated, just snuck these in 🤘

@SimenB
Copy link
Member Author

SimenB commented Oct 6, 2020

@noomorph would love for you to confirm

  1. it fixes the regression without code changes on detox's side
  2. you're able to use runtimeGlobals over this.global.*

I'm done changing the code in this PR now, so if you can confirm those I'll merge and make a patch release ASAP 🙂

@noomorph
Copy link
Contributor

noomorph commented Oct 6, 2020

@SimenB, yes, I've been able to confirm both your statements:

  1. your changes make the existing codebase work as usual.
  2. it is fully possible to combine injectGlobals: false and Object.assign(this.global, event.runtimeGlobals); in TestEnvironment class' setup event handler, and achieve the very same result.

Thanks much again! Looking forward to the release!

@SimenB
Copy link
Member Author

SimenB commented Oct 6, 2020

Perfect, thank you!

@SimenB SimenB merged commit d49cb30 into jestjs:master Oct 6, 2020
@SimenB SimenB deleted the circus-setup-globals branch October 6, 2020 10:20
@SimenB
Copy link
Member Author

SimenB commented Oct 6, 2020

@noomorph out in https://github.com/facebook/jest/releases/tag/v26.5.2

@noomorph
Copy link
Contributor

noomorph commented Oct 6, 2020

@SimenB, excellent. We'll provide a user-friendly warning for Detox users if anyone ever encounters 26.5.0-26.5.1 versions.

@noomorph
Copy link
Contributor

noomorph commented Oct 6, 2020

See: wix/Detox#2391

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants