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

write generated types to __types directories #4705

Merged
merged 10 commits into from
May 19, 2022
Merged

write generated types to __types directories #4705

merged 10 commits into from
May 19, 2022

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Apr 22, 2022

Changes where generated types are written to:

-import type { RequestHandler } from './foo';
+import type { RequestHandler } from './__types/foo';

though the fix seems a bit... unidiomatic? would love a gut check from typescript-heads. little surprised this didn't come up sooner

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Apr 22, 2022

🦋 Changeset detected

Latest commit: 82d85b1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
create-svelte Patch
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member Author

If we do end up doing this, i wonder if we should make the same change to the JSDoc comments, and the examples in the docs:

-/** @type {import('./index').RequestHandler} */
+/** @type {import('./index.d').RequestHandler} */

It works either way, but there might be some value in consistency

@coryvirok
Copy link
Contributor

coryvirok commented Apr 23, 2022

I'm not a Typescript expert, but Microsoft seems to think having files with the same name, (e.g. foo.d.ts and foo.ts) is a bad idea - microsoft/TypeScript#7624

Maybe one solution would be to put the generated types file in types/foo.d.ts?

Tangentially related, I'd like to have my *.d.ts files in src/types/ and I see that the generated subtree of types follows this convention. Meaning, the generated type declarations are put in .svelte-kit/types/. So maybe it's an opportunity to move app.d.ts into src/types/app.d.ts? That could clean up the conventions on where Svelte is storing types and also solve this issue because generated types would no longer be in src/routes/foo.d.ts and would instead be in src/types/routes/foo.d.ts.

@dummdidumm
Copy link
Member

dummdidumm commented Apr 23, 2022

I'm not a Typescript expert, but Microsoft seems to think having files with the same name, (e.g. foo.d.ts and foo.ts) is a bad idea - microsoft/TypeScript#7624

I second this. The TS resolution algorithm has a certain order to resolve files (ts>dts>js, this is why the solution in this PR works because it first appends the ts ending) and I think it wasn't intended to support two files which only differ by file ending. The more robust fix would therefore probably be to create a type file name that is distinct, for example taking advantage of our "double underscore is reserved": import("./__types.index"), or as a folder import("./__types/index"). If we don't want that, the given solution works, (ab)using the TS file ending resolution order.

@ignatiusmb
Copy link
Member

I third it.

@benmccann
Copy link
Member

I suppose I'd lean towards:

/** @type {import('./__types/index').RequestHandler} */

@Rich-Harris Rich-Harris changed the title import generated types from .d.ts file - write generated types to __types directories May 18, 2022
@Rich-Harris
Copy link
Member Author

Updated to use __types, this is now ready for review

@orta
Copy link

orta commented May 18, 2022

Yeah, this fits

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.

Looks good to me apart from the seemingly unrelated packaging changes

packages/kit/src/packaging/test/watch/src/lib/b.ts Outdated Show resolved Hide resolved
/**
* @type {import('./foo').Foo}
*/
export let foo;
Copy link
Member

Choose a reason for hiding this comment

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

These also seem unrelated, but possibly ok to keep because else prettier complains (?)

@Rich-Harris Rich-Harris merged commit 0cabe77 into master May 19, 2022
@Rich-Harris Rich-Harris deleted the gh-4701 branch May 19, 2022 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants