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

Upgrade to TypeScript 3.9 #1267

Merged
merged 10 commits into from
Jul 5, 2020
Merged

Upgrade to TypeScript 3.9 #1267

merged 10 commits into from
Jul 5, 2020

Conversation

sindresorhus
Copy link
Owner

No description provided.

@sindresorhus
Copy link
Owner Author

I think this is caused by microsoft/TypeScript#38540

@BenSjoberg
Copy link

BenSjoberg commented May 27, 2020

The linked TypeScript bug also causes the current version of got to fail when using TS 3.9. The issue occurs when you mix the default import with type imports (i.e. import got, { HTTPError } from 'got') or when using a dynamic import (i.e. await import('got'))

A workaround I found is to add module.exports.__esModule = true after this line. TypeScript sets __esModule to true on the exported module by default, but setting module.exports = got overwrites it.

The right answer might be to just wait for TypeScript to fix the problem, but no one from the team has commented on the bug yet, so it might be a while. :( (Edit: nvm, looks like someone from the TS team accepted the bug earlier today. So hopefully it'll be fixed soon!)

@DanielRosenwasser
Copy link

We should have a fix at microsoft/TypeScript#38808. If you can pick up the build that will be produced at the bottom of the page and try it out, that'd really help give us some confidence that we can back-port it to 3.9.

@sindresorhus
Copy link
Owner Author

I tried it in d6dae82, and seems like it doesn't fix our issue, but it doesn't cause any other problems either.

source/core/index.ts Outdated Show resolved Hide resolved
source/core/index.ts Outdated Show resolved Hide resolved
source/core/index.ts Outdated Show resolved Hide resolved
source/create.ts Outdated Show resolved Hide resolved
@@ -197,6 +198,7 @@ const create = (defaults: InstanceDefaults): Got => {

// Pagination
const paginateEach = (async function * <T, R>(url: string | URL, options?: OptionsWithPagination<T, R>) {
// @ts-expect-error
Copy link
Collaborator

Choose a reason for hiding this comment

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

This too?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I have no idea how to fix it. I just punted it as it's more important to get TS upgraded:

source/create.ts:201:51 - error TS2345: Argument of type 'Merge<Options, PaginationOptions<T, R>> | undefined' is not assignable to parameter of type 'Options | undefined'.
  Type 'Merge<Options, PaginationOptions<T, R>>' is not assignable to type 'Options | undefined'.
    Type 'Merge<Options, PaginationOptions<T, R>>' is not assignable to type 'Options'.
      Types of property 'pagination' are incompatible.
        Type '{ transform?: ((response: Response<R>) => T[] | Promise<T[]>) | undefined; filter?: ((item: T, allItems: T[], currentItems: T[]) => boolean) | undefined; ... 4 more ...; stackAllItems?: boolean | undefined; } | undefined' is not assignable to type '{ transform?: ((response: Response<unknown>) => unknown[] | Promise<unknown[]>) | undefined; filter?: ((item: unknown, allItems: unknown[], currentItems: unknown[]) => boolean) | undefined; paginate?: ((response: Response<unknown>, allItems: unknown[], currentItems: unknown[]) => false | Options) | undefined; should...'.
          Type '{ transform?: ((response: Response<R>) => T[] | Promise<T[]>) | undefined; filter?: ((item: T, allItems: T[], currentItems: T[]) => boolean) | undefined; ... 4 more ...; stackAllItems?: boolean | undefined; }' is not assignable to type '{ transform?: ((response: Response<unknown>) => unknown[] | Promise<unknown[]>) | undefined; filter?: ((item: unknown, allItems: unknown[], currentItems: unknown[]) => boolean) | undefined; paginate?: ((response: Response<unknown>, allItems: unknown[], currentItems: unknown[]) => false | Options) | undefined; should...'.
            Types of property 'transform' are incompatible.
              Type '((response: Response<R>) => T[] | Promise<T[]>) | undefined' is not assignable to type '((response: Response<unknown>) => unknown[] | Promise<unknown[]>) | undefined'.
                Type '(response: Response<R>) => T[] | Promise<T[]>' is not assignable to type '(response: Response<unknown>) => unknown[] | Promise<unknown[]>'.
                  Types of parameters 'response' and 'response' are incompatible.
                    Type 'Response<unknown>' is not assignable to type 'Response<R>'.
                      Type 'unknown' is not assignable to type 'R'.
                        'R' could be instantiated with an arbitrary type which could be unrelated to 'unknown'.

201  	let normalizedOptions = normalizeArguments(url, options, defaults.options);
     	                                                ~~~~~~~


Found 1 error.

I'll add a TODO comment.

Copy link
Owner Author

Choose a reason for hiding this comment

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

TS 3.9 introduced a lot of issues, and it seems many will be fixed in TS 4, so I suggest ignoring it until then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Simplest repro:

interface Options<T = unknown> {
    fn: (value: T) => void
}

const normalizeOptions = (options: Options): void => {};

function someOtherFunction<T>(options: Options<T>) {
    normalizeOptions(options);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Replacing unknown with any seems to fix the issue.

Copy link
Collaborator

@szmarczak szmarczak Jul 5, 2020

Choose a reason for hiding this comment

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

Interestingly the example reproduces the error on all TS 3.x versions.

Copy link
Collaborator

@szmarczak szmarczak left a comment

Choose a reason for hiding this comment

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

Hey, you precious wizard! How have you achieved that? :P

@sindresorhus sindresorhus merged commit b51d836 into master Jul 5, 2020
@sindresorhus sindresorhus deleted the ts39 branch July 5, 2020 21:19
szmarczak pushed a commit to jaulz/got that referenced this pull request Jul 6, 2020
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

4 participants