-
Notifications
You must be signed in to change notification settings - Fork 44
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
Use jsdom polyfill and update jest #2386
Conversation
4a227c5
to
6c4e9dd
Compare
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ecbf668:
|
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.
Looks good overall, a few minor comments
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.
Let's get the changes needed into jest-jsdom-polyfills instead of using a custom environment here.
jest.setup.ts
Outdated
@@ -0,0 +1 @@ | |||
import "@inrupt/jest-jsdom-polyfills"; |
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.
import "@inrupt/jest-jsdom-polyfills"; | |
import "@inrupt/jest-jsdom-polyfills"; | |
missing trailing new line, and eslint may complain about license header.
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.
We currently don't run eslint on config files (none of them include the license header). Should we?
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.
Debatable? It does run in some repos, needs to be standardised as included or ignored in style configs
(this.global.crypto.subtle as any) = (new Crypto()).subtle; | ||
this.global.CryptoKey = CryptoKey; | ||
} | ||
}; |
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.
Let's fix this in jest-jsdom-polyfills instead of using custom environments.
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.
This works in the polyfill (I have tested it locally, I was planning on merging this and revisiting after releasing the updated polyfills), but I couldn't get the Uint8Array
thing to work in there, so for now I'm afraid the custom environment has to stay. We can revisit why, but I'd like to move on because this has been a time sink :)
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.
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.
Hm, okay; I suspect this is due to there being that JSDom UInt8Array issue
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 think this should've been released now, so this shouldn't be necessary anymore.
Instead of relying on Lerna to run tests across the repos, just use Jest picking a specific project or running them all.
Initially, the intent of this PR was just to use jsdom polyfills we provide to simplify the environment setup. However, it showed that we are currently not running our unit test using the browser jose bundle in the browser package, because we are using an older version of
jest
. So this PR also looks at updating jest to the latest version.EDIT: a bunch of upgrades tacked on this PR:
ts-jest