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

fix(typescript): add missing imports #633

Merged
merged 2 commits into from Nov 30, 2020
Merged

fix(typescript): add missing imports #633

merged 2 commits into from Nov 30, 2020

Conversation

apfelbox
Copy link
Contributor

@apfelbox apfelbox commented Nov 3, 2020

Rollup Plugin Name: TypeScript

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

I have no idea how to test this, as we would need to run the build with both versions of TypeScript and I don't know if sth like this is already set up for this repository).

List any relevant issue numbers:

Description

These two imports were missing and fail the TypeScript build of the Typescript rollup plugin with TypeScript 4.0+
Not sure if they failed for ^3.9 as well.

I tested this change with both TypeScript 3.9.7 and 4.0.5 and it works.
Doesn't seem to fix the last issue of #626 though.

This imports fail the build right now, and I checked the types with TypeScript 3.9 and 4.0 and they still work.
@apfelbox apfelbox changed the title Add missing imports [TypeScript] Add missing imports Nov 3, 2020
@apfelbox
Copy link
Contributor Author

apfelbox commented Nov 3, 2020

The second issue is here:

type ElementType<T extends Array<any>> = T extends (infer U)[] ? U : never;
                           ^^^^^^^^^^

type StagedTransformerFactory<T extends TransformerStage> = ElementType<CustomTransformers[T]>;
                                                                        ^^^^^^^^^^^^^^^^^^^^^

Because as every property of CustomTransformers can be undefined, CustomTransformers[T] is the defined type|undefined.

And therefore the type tries to assign undefined to T extends any[], which doesn't work.

There are two possible fixes, but I am (semantically) not sure, which one is the better fit.

Solution 1: allow undefined in ElementType

- type ElementType<T extends Array<any>> = T extends (infer U)[] ? U : never;
+ type ElementType<T extends (Array<any>|undefined)> = T extends (infer U)[] ? U : never;

Solution 2: making all properties required, before fetching them

- type StagedTransformerFactory<T extends TransformerStage> = ElementType<CustomTransformers[T]>;
+ type StagedTransformerFactory<T extends TransformerStage> = ElementType<Required<CustomTransformers>[T]>;

@shellscape shellscape changed the title [TypeScript] Add missing imports fix(typescript): add missing imports Nov 3, 2020
@apfelbox
Copy link
Contributor Author

apfelbox commented Nov 5, 2020

If somebody can give me feedback on the question in #633 (comment), I can add it (and we can get this merged)

@shellscape
Copy link
Collaborator

@apfelbox I'm probably not as learned as you with TS, but could using ElementType<Partial<CustomTransformers[T]>> work here?

@apfelbox
Copy link
Contributor Author

@shellscape I don't think that that would work. Partial makes every property in an interface optional (e.g. {test: string} -> {test?: string}), but they already are all optional (they all already have a ?:).

That is the main problem, actually: as they are optional, the type of the index access in CustomTransformers[T] can be undefined, which is then in turn not accepted in the definition of ElementType itself.

@shellscape
Copy link
Collaborator

@apfelbox does #647 resolve the issue on your end? Seems to be a possible duplication of effort.

@apfelbox
Copy link
Contributor Author

@shellscape yes, I the meantime #647 popped up and seems to be fixing the same issues.
However I would argue (and have in an comment over there) that one of the fixes is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants