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

chore: update to [email protected] #10859

Merged
merged 4 commits into from
Feb 18, 2021
Merged

chore: update to [email protected] #10859

merged 4 commits into from
Feb 18, 2021

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Nov 23, 2020

Summary

I'm unsure about the second commit - seems to be some array changes in TS 4.1 that are causing errors for us. @jeysal thoughts?

Updating to 4.1.5 fixed those errors

Test plan

Green CI

Copy link
Contributor

@ahnpnl ahnpnl left a comment

Choose a reason for hiding this comment

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

The type cast looks ok to me, just curious why have to do it explicitly. Does tsc complain without explicit casting ?

@@ -101,7 +101,9 @@ export const _replaceRootDirTags = <T extends ReplaceRootDirConfigValues>(
case 'object':
if (Array.isArray(config)) {
/// can be string[] or {}[]
return config.map(item => _replaceRootDirTags(rootDir, item)) as T;
return (config as Array<string>).map(item =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit wrong comparing to the comment above it

@@ -26,6 +26,7 @@ export default class PCancelable<T> extends Promise<T> {
reject: (reason?: unknown) => void,
) => void,
) {
// @ts-expect-error
Copy link
Member Author

Choose a reason for hiding this comment

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

@G-Rath I had to add these ignores now (from #10215). Any ideas on how to fix it without the ignore?

Copy link
Contributor

Choose a reason for hiding this comment

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

What are the actual errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

hah, should have pushed without the ignore 🙈

packages/jest-jasmine2/src/PCancelable.ts:29:22 - error TS2554: Expected 1 arguments, but got 0.

29     super(resolve => resolve());
                        ~~~~~~~~~

  node_modules/typescript/lib/lib.es2015.promise.d.ts:33:34
    33     new <T>(executor: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void): Promise<T>;
                                        ~~~~~~~~~~~~~~~~~~~~~~~~~
    An argument for 'value' was not provided.

packages/jest-jasmine2/src/PCancelable.ts:40:19 - error TS2345: Argument of type 'T | PromiseLike<T> | undefined' is not assignable to parameter of type 'T | PromiseLike<T>'.
  Type 'undefined' is not assignable to type 'T | PromiseLike<T>'.

40           resolve(val);
                     ~~~

[8:25:19 PM] Found 2 errors. Watching for file changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

probably related to the new Promise<void> changes I've had to make other places, but I wasn't able to make this one happy

Copy link
Contributor

@G-Rath G-Rath Feb 18, 2021

Choose a reason for hiding this comment

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

I think the answer for that last one is that we need to change resolve in the same way TS did in 4.1:

resolve‘s Parameters Are No Longer Optional in Promises

So resolve: (value?: T | PromiseLike<T>) => void, needs to lose its ?.

Copy link
Contributor

@G-Rath G-Rath Feb 18, 2021

Choose a reason for hiding this comment

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

Maybe replace extends Promise<T> with implements PromiseLike<T> - that doesn't give errors for me in this file but I've not checked if the rest of the project is typesafe.

We might have to chuck the setPrototypeOf back for instanceof and such to work?

Copy link
Member Author

@SimenB SimenB Feb 18, 2021

Choose a reason for hiding this comment

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

If it's easier we can just use some function instead of new PCancelable which extends Promise?

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 don't need instanceof, this is just internal to have a cancel

Copy link
Contributor

Choose a reason for hiding this comment

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

That'd also probably work 🤷

This is only used in jest-jasmine2 internally, so shouldn't be too much work to refactor nor too breaking.

I'd see if it compiles & passes the tests with doing implements PromiseLike<T> first as that to me feels like the right change, and I think that should be fine as the only place this is actually used is in queneRunner.

That function returns an object with cancel: () => void; - while it binds the cancel to the PCancelable instance, there's no spec saying it's this is a Promise nor does it return a Promise for chaining.

So I think it should all be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

implements PromiseLike<T> seems to work, yeah 🚀

@SimenB SimenB merged commit 66629be into jestjs:master Feb 18, 2021
@SimenB SimenB deleted the ts-4.1 branch February 18, 2021 20:51
@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 10, 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