-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature: Allow use with injectGlobals: false
#182
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the MR @cprecioso! 💜
I have left some initial thoughts on a few lines about compatibility. Feel free to look into them if you'd like. I should probably get familiar with them too.
If it ends up being a breaking change, I'll may include this alongside the window.history
support added in #179 which is right around the corner 😄
// eslint-disable-next-line @typescript-eslint/triple-slash-reference, spaced-comment | ||
/// <reference path="../jest.d.ts" /> | ||
// eslint-disable-next-line import/namespace | ||
import {expect} from "@jest/globals"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if this would break support for those who don't use the globals import. Or maybe more likely, break support with the Jest versions that don't have a globals import in the core jest package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self, this MR will and the alpha version with window.history support conflict in that we may need to add this import to more locations.
@@ -46,6 +46,7 @@ | |||
}, | |||
"dependencies": { | |||
"@jedmao/location": "^3.0.0", | |||
"@jest/globals": "^29.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if this locks this package into supporting Jest v29 until we update this to v30 when it comes around and so forth. Maybe this should be a peerDependency. This may need investgation.
Jest has an
injectGlobals: false
option that forces you to import the global functions from@jest/globals
, this PR makes it compatible from that option by always importing it from there, which would work in both cases.