-
-
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
fix: correctly include ambient types from adapters #12088
Conversation
…torage type import cannot coexist with it
🦋 Changeset detectedLatest commit: 6242385 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 |
Inclusion of the Svelte config was reverted in #11908 because it's a breaking change strictly speaking. Is it ok to hold off from doing this until 3.0? |
Oh whoops. I didn’t realise it was something we already tried to do before. Yeah, I think it’s fine to wait for 3.0 for this. It has an easy workaround (just include the types or reference the adapter in a declaration file). Maybe I can split the ambient declaration changes and the svelte config inclusion into separate PRs? |
note: it was suggested to not include the cloudflare types because they pollute the ambient types namespace. |
@benmccann is this a breaking change because of the additional types in the ambient namespace? |
Oh, I was confused because of the prior conversation on this PR (which seems to refer to a change that was dropped from this PR and moved to #12090) and because this PR was in the 3.0 milestone. I've taken it off the 3.0 milestone and removed the breaking change label |
@@ -1,3 +1,5 @@ | |||
/// <reference types="@cloudflare/workers-types" /> |
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.
cc: @gtm-nayan do you still think we should omit the worker types reference here to avoid polluting the ambient namespace?
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.
Personally yes, but I should note that my usage of Cloudflare is only limited to a couple of projects so we could get more empirical.
Are the declarations limited to types or do they also add global variable and module declarations?
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'm not entirely sure myself. @dario-piotrowicz what do you think about this? Are there any other frameworks doing this?
As an aside, I'm already getting them as ambient types because I have the drizzle orm d1 driver imported and they're doing this.
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'm removing them from the ambient file since our docs already suggest to import them directly. https://svelte.dev/docs/kit/adapter-cloudflare#Runtime-APIs
preview: https://svelte-dev-git-preview-kit-12088-svelte.vercel.app/ this is an automated message |
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.
looks good except one question
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
…js/kit into fix-adapter-ambient-types
part of closing #11731closes #12433
While investigating #11731 , I found that the ambient types from adapters weren't showing up when I tried installing the cloudflare, vercel, and node adapters in a fresh project and tried to access the autocomplete on
event.platform
. This is dependent on #12090 being merged first to get the types to show up in the IDE autocomplete.This PR does the following:
* includes theEDIT: separated into #12090svelte.config.js
file in the generated tsconfig so that when the svelte.config.js file imports the adapter, the ambient types are included for the whole project.* references the worker types in the cloudflare adapters so that when the adapter is used, the project can access those types.files
array so users can get those types included automatically for theirevent.platform
.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
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits