fix(jest-environment-puppeteer): Class & Exports #37647
Conversation
This makes the typings actually compatible to create your own test environment and not just as part of the bigger jest-puppeteer package. How to do this is pretty much what the test covers. One of the reasons to do this is to write a handler where Puppeteer takes screenshots after each failed test, as discussed [here](argos-ci/jest-puppeteer#131)
|
👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics. Let’s review the numbers, shall we?
Looks like there were a couple significant differences—take a look at worst-case duration for getting quick info at a position to make sure everything looks ok. If you have any questions or comments about me, you can ping |
|
@favna Thank you for submitting this PR! 🔔 @JoshuaKGoldberg @ifiokjr - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
| // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
| // TypeScript Version: 2.8 | ||
| // TypeScript Version: 3.4 |
There was a problem hiding this comment.
Why the version change? This would be a breaking change.
There was a problem hiding this comment.
this would be a breaking change
Hardly, if this extremely small set of breaking changes made by TS v3.0 is an issue then you should update your code base rather than resist upgrading.
But to answer your question, before I bumped it the tests failed, something to do with the imports the new tests and typings have to use.
There was a problem hiding this comment.
It's not always reasonable to ask consumers to upgrade TypeScript versions for their projects especially if it's not necessary. In the case of this PR I think the esModuleInterop, which is only used for the tests, is causing the requirement for a higher version. Please remove this flag and use import JestEnvironmentPuppeteer = require('jest-environment-puppeteer') instead.
There was a problem hiding this comment.
@PranavSenthilnathan I tried bumping it back down to 2.8 and removing esModuleInterop and I got the errors again, it comes from the jest-mock library. Here is the error it throws:
1> jest-environment-puppeteer failing:
1> Error: Errors in typescript@next for external dependencies:
1> node_modules/@jest/environment/build/index.d.ts(11,8): error TS1259: Module '"E:/dev/DefinitelyTyped/types/jest-environment-puppeteer/node_modules/jest-mock/build/index"' can only be default-imported using the 'esModuleInterop' flag
1> node_modules/@jest/source-map/build/getCallsite.d.ts(8,100): error TS2503: Cannot find namespace 'callsites'.
1>
1> at E:\dev\DefinitelyTyped\node_modules\dtslint\bin\index.js:190:19
1> at Generator.next (<anonymous>)
1> at fulfilled (E:\dev\DefinitelyTyped\node_modules\dtslint\bin\index.js:5:58)
Going back to either 3.0 or 3.4 without esModuleInterop still has it fail, so instead I tried version 2.8 with esModuleInterop and I ended up with another error, once again due to external dependencies, this is a long one so I dumped it on gist: https://gist.github.com/Favna/fb37f696dcc390fa593cedcf8050751b.
Next thing I tried was TypeScript v3.0 with esModuleInterop which again threw an error:
=== ERRORS ===
Error in jest-environment-puppeteer
Error: Errors in typescript@3.0 for external dependencies:
node_modules/jest-mock/build/index.d.ts(125,162): error TS2304: Cannot find name 'Parameters'.
at E:\dev\DefinitelyTyped\node_modules\dtslint\bin\index.js:190:19
at Generator.next (<anonymous>)
at fulfilled (E:\dev\DefinitelyTyped\node_modules\dtslint\bin\index.js:5:58)
Error: The following packages had errors: jest-environment-puppeteer
at doRunTests (E:\dev\DefinitelyTyped\node_modules\types-publisher\src\tester\test-runner.ts:235:11)
at processTicksAndRejections (internal/process/task_queues.js:86:5)
error Command failed with exit code 1.
Ultimately I ended up going for TypeScript version 3.4 in 53b225a after all and with esModuleInterop, but I maintained the requested export = and import .. = require('...'). I also added a few extra ExpectType tests because the latter means you cannot directly import the interfaces and I'm told I can't have other export statements when usin export =.
There was a problem hiding this comment.
Its unfortunate that the packages you are depending on are using features from typescript 3+.
typescript-bot
moved this from Waiting for Reviewers
to Check and Merge
in Pull Request Status Board
typescript-bot
added
Owner Approved
|
A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
| // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
| // TypeScript Version: 2.8 | ||
| // TypeScript Version: 3.4 |
There was a problem hiding this comment.
It's not always reasonable to ask consumers to upgrade TypeScript versions for their projects especially if it's not necessary. In the case of this PR I think the esModuleInterop, which is only used for the tests, is causing the requirement for a higher version. Please remove this flag and use import JestEnvironmentPuppeteer = require('jest-environment-puppeteer') instead.
typescript-bot
moved this from Check and Merge
to Needs Author Attention
in Pull Request Status Board
typescript-bot
added
Revision needed
|
@favna One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you! |
Co-Authored-By: Pranav Senthilnathan <pranav.senthilnathan@live.com>
|
Updated numbers for you here from 5ac3547: Comparison details 📊
Looks like there were a couple significant differences—take a look at worst-case duration for getting quick info at a position to make sure everything looks ok. |
|
Updated numbers for you here from 5ac3547: Comparison details 📊
It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place. |
|
Updated numbers for you here from 9bb675f: Comparison details 📊
It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place. |
|
🔔 @PranavSenthilnathan - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
|
I just published |
This makes the typings actually compatible to create your own test environment and not just as part of the bigger jest-puppeteer package. How to do this is pretty much what the test covers. One of the reasons to do this is to write a handler where Puppeteer takes screenshots after each failed test, as discussed here
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.