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: remove internal __sveltekit/ module declarations from types #11620

Merged
merged 6 commits into from Jan 12, 2024

Conversation

dummdidumm
Copy link
Member

Takes advantage of the fact that dts-buddy doesn't detect the ambient-private.d.ts module declarations (which arguably is a bit weird and could result in buggy behavior, but we can use it to our advantage here).

My guess why this happens: dts-buddy doesn't pick up ambient-private.d.ts because it's not using the includes field of the tsconfig.json and instead only follows the paths it knows from imports - in the case of __sveltekit/x it doesn't know where to go though so it leaves it as is. One could argue that this is the desired/expected behavior. If not, then we would need a mechanism to tell dts-buddy that certain ambient module declarations are private and shouldn't be bundled in unless they are needed (through @internal for example).

fixes #11607


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 pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Takes advantage of the fact that dts-buddy doesn't detect the ambient-private.d.ts module declarations (which arguably is a bit weird and could result in buggy behavior, but we can use it to our advantage here).

fixes #11607
Copy link

changeset-bot bot commented Jan 12, 2024

🦋 Changeset detected

Latest commit: 124adbf

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

This PR includes changesets to release 1 package
Name Type
@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

The test failures look legit, the paths have different values somehow?

@Rich-Harris
Copy link
Member

Ah, of course — it's because the bindings are live. By re-exporting them, they stay live. By aliasing them, we evaluate them eagerly. I think we're going to have to find another approach unfortunately.

@Rich-Harris
Copy link
Member

One idea — we could use .d.ts files as entry points for dts-buddy instead of .js files. Doing something similar in #11406

@dummdidumm
Copy link
Member Author

Ah that's a gnarly gotcha - yeah I guess we have to use d.ts entry points then.

@dummdidumm dummdidumm marked this pull request as draft January 12, 2024 13:47
@dummdidumm
Copy link
Member Author

#11406 (comment) this could be another way solve this - use a path alias in our tsconfig to resolve to a d.ts file

@Rich-Harris
Copy link
Member

My guess why this happens: dts-buddy doesn't pick up ambient-private.d.ts because it's not using the includes field of the tsconfig.json and instead only follows the paths it knows from imports

Yeah, this is right. I think that's the correct behaviour though — if you were using tsc -d instead of dts-buddy those ambient declarations still wouldn't be visible to consumers unless you went out of your way to make them visible, IIUC

@Rich-Harris
Copy link
Member

I went with the facade .d.ts approach rather than an alias, because the alias would have to point at something in .svelte-kit. It's more work to create that file, and another thing that can't typecheck until you run sync, so even though there's a small bit of duplication between index.js and types.d.ts in the app/paths and app/environment directories, it feels like the nicest approach

@Rich-Harris Rich-Harris marked this pull request as ready for review January 12, 2024 21:42
@Rich-Harris Rich-Harris merged commit 2137717 into main Jan 12, 2024
13 checks passed
@Rich-Harris Rich-Harris deleted the omit-internal-module-declarations branch January 12, 2024 21:46
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
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.

Sveltekit internal paths import suggestions
2 participants