fix(typescript): type declarations #647

geigerzaehler · 2020-11-13T17:46:10Z

Rollup Plugin Name: `typescript`

This PR contains:

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

~~If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.~~

List any relevant issue numbers:

Description

Before, using type definitions from this package without the skipLibCheck option for TypeScript would result in type checking errors. We fix this and to ensure that the types stay correct we have the checked as part of the build process by disabling the skipLibCheck option.

shellscape · 2020-11-28T02:56:13Z

Thanks for opening this PR 🎉

We've got one similar in #633 so I'd like to give the author of that one time to weigh in. Hang in there, we'll get this resolved soon :)

apfelbox · 2020-11-30T07:40:39Z

packages/typescript/types/index.d.ts

@@ -41,7 +41,7 @@ export interface RollupTypescriptPluginOptions {
 type ElementType<T extends Array<any>> = T extends (infer U)[] ? U : never;

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


This seems to be an error. This change means that it isn't the list of a single transform step anymore, but an array of an array of transforms.

The type of CustomTransformers is:

export interface CustomTransformers { /** Custom transformers to evaluate before built-in .js transformations. */ before?: (TransformerFactory<SourceFile> | CustomTransformerFactory)[]; /** Custom transformers to evaluate after built-in .js transformations. */ after?: (TransformerFactory<SourceFile> | CustomTransformerFactory)[]; /** Custom transformers to evaluate after built-in .d.ts transformations. */ afterDeclarations?: (TransformerFactory<Bundle | SourceFile> | CustomTransformerFactory)[]; }

So CustomTransformers[T] already is an array. Making another array from it, removes the error but is probably semantically incorrect.

See #633 (comment) for a discussion of alternative solutions.

I wasn’t quite sure what was happening here so I just tried to make it work. I’ve used the first solution you suggested to fix the issue.

apfelbox · 2020-12-01T10:08:41Z

@shellscape this one looks ready as well and should fix all TypeScript errors 👍

benmccann · 2020-12-07T22:07:20Z

packages/typescript/tsconfig.json

-  "include": ["src/**/*", "types/**/*"]
+  "include": ["src/**/*", "types/**/*"],
+  "compilerOptions": {
+    "skipLibCheck": false


I think this is the default, so you shouldn't really need to change it here

https://www.typescriptlang.org/docs/handbook/compiler-options.html

You’re correct, it is the default for the compiler. But the project tsconfig sets it to true. I tried to be unintrusive and not change it for the whole project.

Ah, I missed that. Though it looks like we can remove it in the base config without any additional changes, so that might be the better way to go: #700

If needed, could we roll back this change in order to merge the change for the type that is breaking in #626 ?

I think the thing to do would be to merge #700 instead of changing this file, but perhaps @shellscape could weigh in on whether we can merge that one

#700 is merged, please @geigerzaehler can you revert this change and rebase your PR?

benmccann · 2020-12-07T22:10:45Z

We fix this and to ensure that the types stay correct we have the checked as part of the build process by disabling the skipLibCheck option

I don't understand how disabling a check would help prevent regressions. What I normally do is rename the .d.ts file to .ts and put it in the source directory and then let @rollup/plugin-typescript generate the .d.ts files

geigerzaehler · 2020-12-08T09:34:42Z

I don't understand how disabling a check would help prevent regressions. What I normally do is rename the .d.ts file to .ts and put it in the source directory and then let @rollup/plugin-typescript generate the .d.ts files

Enabling the lib check will also typecheck the .d.ts that is committed to source and force it to be correct.

I agree with you that normally you would want to avoid hand-written declaration files and instead have the compiler generate them for you. But I don’t know why this decision was made for the repo in the first place and I tried to keep the changes to a minimum.

benmccann · 2020-12-08T16:53:52Z

Enabling the lib check will also typecheck the .d.ts that is committed to source and force it to be correct.

Will it? If that's true then this PR isn't necessary because everything builds and all tests pass with skipLibCheck removed in #700

geigerzaehler · 2020-12-11T11:08:49Z

Will it? If that's true then this PR isn't necessary because everything builds and all tests pass with skipLibCheck removed in #700

Great find. I don’t understand the build and check system for this project and I assumed that it would do something that is equivalent to tsc -p packages/typescript. Running this command fails for me without the fix but I does not seem to be run on CI so I guess that is something that should be enabled.

shellscape · 2021-01-20T01:59:14Z

@geigerzaehler we merged #700, could you take another look at this?

We fix the type declaration file for the `typescript` plugin. Signed-off-by: Thomas Scholtes <thomas-scholtes@gmx.de>

geigerzaehler · 2021-01-20T16:11:45Z

@geigerzaehler we merged #700, could you take another look at this?

Cool. I’ve updated the branch and removed skipLibCheck: false. So this PR now fixes the issue but the CI still doesn’t not do type checking so this might break again. Since I’m not familiar with the CI setup it would make sense to tackle this separately.

fernandopasik · 2021-01-20T17:57:40Z

Looking at https://github.com/rollup/plugins/blob/master/.circleci/config.yml I don't see any task to do typescript type checks

Each project in the monorepo has the task test:ts which does run tsc and check the types

fernandopasik

In any case that could be an improvement for the CI.
This PR scope is to fix a type issue

shellscape · 2021-01-29T14:14:09Z

@fernandopasik that's correct, plugins should run type checking independently. since the typescript plugin doesn't have that, I'll make a note to add it following merging of this PR. thanks for pointing that out.

fernandopasik · 2021-01-29T15:57:05Z

I'll create an issue and attempt a PR to resolve it.

fernandopasik · 2021-01-29T16:03:19Z

#779

geigerzaehler changed the title ~~## Rollup Plugin Name: typescript~~ Fix type declarations in typescript plugin Nov 13, 2020

shellscape mentioned this pull request Nov 28, 2020

fix(typescript): add missing imports #633

Merged

9 tasks

shellscape requested a review from NotWoods November 28, 2020 02:56

shellscape added pr⁴ 🔧 fix 🔌 plugin-typescript labels Nov 28, 2020

shellscape changed the title ~~Fix type declarations in typescript plugin~~ fix(typescript): type declarations Nov 28, 2020

apfelbox reviewed Nov 30, 2020

View reviewed changes

benmccann reviewed Dec 7, 2020

View reviewed changes

fernandopasik mentioned this pull request Jan 1, 2021

[TypeScript] Build broken due to type issues #626

Closed

fix(typescript): fix library type declrations

5e980f6

We fix the type declaration file for the `typescript` plugin. Signed-off-by: Thomas Scholtes <thomas-scholtes@gmx.de>

geigerzaehler force-pushed the fix-typescript-plugin-types branch from ed2c87d to 5e980f6 Compare January 20, 2021 16:12

fernandopasik approved these changes Jan 20, 2021

View reviewed changes

shellscape requested a review from TrySound January 22, 2021 01:21

shellscape merged commit 181b929 into rollup:master Jan 29, 2021

fernandopasik mentioned this pull request Jan 29, 2021

Check types in typescript packages in CI #780

Merged

9 tasks

geigerzaehler deleted the fix-typescript-plugin-types branch February 1, 2021 09:21

This was referenced Mar 5, 2021

Bump @rollup/plugin-commonjs from 14.0.0 to 17.1.0 erbridge/govuk-prototype-template#2

Closed

chore(deps-dev): bump @rollup/plugin-commonjs from 15.1.0 to 17.1.0 vinayakkulkarni/v-vectors#2

Closed

This was referenced Mar 10, 2021

build: bump @rollup/plugin-commonjs from 16.0.0 to 17.1.0 avolantis/ts-guid#110

Merged

build(deps-dev): bump @rollup/plugin-commonjs from 17.0.0 to 17.1.0 outpostHQ/numl#142

Merged

Bump @rollup/plugin-commonjs from 15.0.0 to 17.1.0 Devcon4/Oriole#9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(typescript): type declarations #647

fix(typescript): type declarations #647

geigerzaehler commented Nov 13, 2020 •

edited

shellscape commented Nov 28, 2020

apfelbox Nov 30, 2020

geigerzaehler Nov 30, 2020

apfelbox commented Dec 1, 2020

benmccann Dec 7, 2020

geigerzaehler Dec 8, 2020

benmccann Dec 8, 2020

fernandopasik Jan 1, 2021

benmccann Jan 1, 2021

fernandopasik Jan 20, 2021 •

edited

benmccann commented Dec 7, 2020

geigerzaehler commented Dec 8, 2020

benmccann commented Dec 8, 2020

geigerzaehler commented Dec 11, 2020

shellscape commented Jan 20, 2021

geigerzaehler commented Jan 20, 2021

fernandopasik commented Jan 20, 2021

fernandopasik left a comment

shellscape commented Jan 29, 2021

fernandopasik commented Jan 29, 2021

fernandopasik commented Jan 29, 2021

fix(typescript): type declarations #647

fix(typescript): type declarations #647

Conversation

geigerzaehler commented Nov 13, 2020 • edited

Rollup Plugin Name: typescript

Description

shellscape commented Nov 28, 2020

apfelbox Nov 30, 2020

Choose a reason for hiding this comment

geigerzaehler Nov 30, 2020

Choose a reason for hiding this comment

apfelbox commented Dec 1, 2020

benmccann Dec 7, 2020

Choose a reason for hiding this comment

geigerzaehler Dec 8, 2020

Choose a reason for hiding this comment

benmccann Dec 8, 2020

Choose a reason for hiding this comment

fernandopasik Jan 1, 2021

Choose a reason for hiding this comment

benmccann Jan 1, 2021

Choose a reason for hiding this comment

fernandopasik Jan 20, 2021 • edited

Choose a reason for hiding this comment

benmccann commented Dec 7, 2020

geigerzaehler commented Dec 8, 2020

benmccann commented Dec 8, 2020

geigerzaehler commented Dec 11, 2020

shellscape commented Jan 20, 2021

geigerzaehler commented Jan 20, 2021

fernandopasik commented Jan 20, 2021

fernandopasik left a comment

Choose a reason for hiding this comment

shellscape commented Jan 29, 2021

fernandopasik commented Jan 29, 2021

fernandopasik commented Jan 29, 2021

geigerzaehler commented Nov 13, 2020 •

edited

Rollup Plugin Name: `typescript`

fernandopasik Jan 20, 2021 •

edited