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

Do not globally generate typings #139

Closed
wants to merge 1 commit into from

Conversation

ehrencrona
Copy link
Contributor

@ehrencrona ehrencrona commented Nov 17, 2020

Having a common tsconfig is great, but I don't believe it should turn on typings generation by default. For the adapters the outcome is generally not what we want. For example, the Netlify adapter puts the typings of render.ts into an index.d.ts that is next to an unrelated index.js. (I notice this is fixed by #138, but anyway, I don't think it's a reasonable default)

So I think we'd better turn it off on a global level and activate it in the subprojects where we actually support it.

As for declarationDir it seems to be ignored by the rollup typescript plugin.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Makes sense, such things are project specific

@benmccann
Copy link
Member

If we were going to turn it off here we should also turn it on in the appropriate subprojects (e.g. packages/app-utils/tsconfig.json)

It seems like a better default to generate the types though. Then there always available if you want to use them and I don't really see any downside to generating them. Is there some con to doing it that I'm overlooking?

The netlify output was strange not just for the types but for the actual .js output as well, which is why I fixed it. That issue seems orthogonal as to whether we generate types or not

The output dir is ignored by default but will be used if you set useTsconfigDeclarationDir. The only subproject that sets it is kit and it has its own tsconfig.json, so I'm fine removing the output dir here. I wonder if we should remove that flag from kit because there's no output dir set in that package, so I'm confused why useTsconfigDeclarationDir was set there

@ehrencrona
Copy link
Contributor Author

ehrencrona commented Nov 17, 2020

The con is mostly that you get empty index.d.ts files in the roots of the projects. As far as I know, only kit and app-utils actually need type generation. Kit carries its own config in the roll up config and on app-utils I set I locally in the PR. Do any of the adapters actually export types?

@ehrencrona
Copy link
Contributor Author

Oh, and about useTsconfigDeclarationDir in kit it’s just that without it it just wasn’t generating types. I commented on that in the PR that introduced it. We could maybe try again to remove it and see if something has changed so it works without it.

@benmccann
Copy link
Member

benmccann commented Nov 21, 2020

Yeah index.d.ts is empty though the others like render.d.ts and server.d.ts do have some contents.

Removing the declarationDir here probably makes sense in either case

@benmccann
Copy link
Member

I sent #167 to remove useTsconfigDeclarationDir and do some other cleanup

@ehrencrona ehrencrona closed this Nov 21, 2020
@Conduitry Conduitry deleted the no-global-typings-generation branch April 17, 2021 03:12
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.

3 participants