-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
add a SvelteKit namespace for app-level types (#3569) #3670
Conversation
🦋 Changeset detectedLatest commit: 52b31ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
✔️ Deploy Preview for kit-demo canceled. 🔨 Explore the source changes: 52b31ee 🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61faa2b3349ddd0008ae71e1 |
How helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this change, especially because it allows us to remove so many type-level code and the generics hack.
Couple of thoughts:
- is
app.d.ts
the best name? You can add anything that is global/ambient there, I slightly prefer the previous name, but I'm not passionate about it and can live with it either way - Is
App
a good namespace name? My fear is that it's rather generic and the possibility of namespace clashes might be higher than with another name. What about$App
since that somewhat corresponds to all the$app/..
imports? - Are all of these globally valid? For example
Stuff
does differ depending on your route. You might havefoo
insidestuff
in one part of your app andbar
in another part. This is not possible to express with this way - but I don't know how we could express this either. The only way I can think of is people wrapping thestuff
store and reexporting it with a more specific type likeexport const myStuff = stuff as Writable<SomeSubStuff>
interface Locals {} | ||
interface Platform {} | ||
interface Session {} | ||
interface Stuff {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any possibility that someone would want this to be a primitive type which can't be expressed as an object-like interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they're all required to be objects
Ultimately it doesn't really matter — you could rename it, or create a
Would those clashes ever be coming from dependencies? I haven't ever seen that happen but maybe there are packages that do that? If it's only likely to conflict with the user's own namespaces then I think that's probably fine, they would just need to rename stuff (and even then, only if their own namespace contained interfaces with conflicting names).
It can differ, but since it's globally accessible as |
As soon as something make TS load a namespace it's part of the global types, and they can then clash. Doesn't matter where they come from. They also do need to have the same interface definitions inside for the clash to be noticeable though, which I guess would be very rare. I don't know, maybe this fear is unfounded.
👍
Agree that the benefits outweigh the edge cases of pedantic typing wizards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is refreshing to see. adapter-cloudflare
docs needs updating as well, but other than that, this looks good!
Add App namespace for app-level types [#3670](sveltejs/kit#3670)
* Update $app/stores page.stuff to use App.Stuff Following #3670, `page.stuff` from `$app/stores` should use `App.Stuff` instead of the old `Record<string, any>` * Create sharp-beers-know.md Co-authored-by: Rich Harris <hello@rich-harris.dev>
Ref #3569.
This is a breaking change, as it affects how
Load
andErrorLoad
are typed if you're using generic arguments. If you're not, TypeScript will likely start yelling at you becausesession
andlocals
etc are no longer typed asany
.By creating an app-level
namespace
, we're able to provide types forevent.locals
,event.platform
,page.session
andpage.stuff
throughout the app with no duplication. One caveat — it's no longer possible to specify inputstuff
and outputstuff
separately for aload
function (unless you create your own type, I suppose) which might be a nuisance if you're ultra-pedantic. Personally I think the ergonomics are vastly better this way.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpx changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0