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

feat: add @jest/globals for schematics #1324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Yehonal
Copy link

@Yehonal Yehonal commented Feb 21, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #1942

What is the new behavior?

This PR adds the jest/globals for every generated file

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@micalevisk
Copy link
Member

micalevisk commented Feb 21, 2023

I wonder if wouldn't be better if we only add those import statements if injectGlobals is false, although I personally prefer having them regardless of that flag

@Yehonal
Copy link
Author

Yehonal commented Feb 21, 2023

I'm not sure there's an easy way to know it, considering that jest can be configured with a javascript file that can be dynamic, and we probably want to avoid running it to discover what config the user has.

What would be the main motivation to not use this import?

@micalevisk
Copy link
Member

micalevisk commented Feb 21, 2023

What would be the main motivation to not use this import?

not having those globals isn't common (not yet, at least). And the jest docs has a note regarding that injectGlobals flag:

This option is only supported using the default jest-circus test runner.

I'm unsure on what that means, in practice tho.

The only way I can think of is by adding yet another flag to the schematics to opt-in to this change. Then the CLI would define the value of that flag at runtime. Since the jest config file can be defined by the end user (ie, it could have any name), this solution won't work for everyone.


But if Kamil agree that we could start having this import, it's fine. I like this new version more.

@Yehonal
Copy link
Author

Yehonal commented Feb 21, 2023

That comment means that if you do not use jest-circus, you are forced to import those globals because they will never be injected, even with the injectGlobals: true

I guess that this import will always work, no matter the value of the config or what runner is used.

@kamilmysliwiec
Copy link
Member

Is Jest about to drop the auto-globals-injection feature any time soon?

@Yehonal
Copy link
Author

Yehonal commented Feb 22, 2023

Is Jest about to drop the auto-globals-injection feature any time soon?

I'm not sure about that, but there are already certain cases where the globals are not injected anymore (when the runner is different or when you have ESM enabled) that's also why I discovered it.
In any case, I think it's always better to not pollute the global scope anyway.

@micalevisk
Copy link
Member

Do those imports works when injectGlobals is true?

@Yehonal
Copy link
Author

Yehonal commented Feb 22, 2023

Do those imports works when injectGlobals is true?

Yes

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.

None yet

3 participants