-
-
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
(WIP) introduce a way for users to emulate the platform
object for local development
#11323
Conversation
|
This would be great |
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 feels like the wrong API to me. I think it'd be better if the adapter could be used to provide the platform
in dev mode as well
Yeah, I agree, this doesn't feel like the right API. Until we figure out how we want to have adapters affect dev mode, I think the best option is to have a |
@benmccann are saying that you'd be ok with this approach if I moved the |
As I also mentioned in the PR description, I do agree with you saying that there should probably be a better solution on how adapters should effect dev mode as a whole, what I am questioning here is: when is that solution going to be implemented? any time soon? if not, wouldn't it be nice to provide a valid stopgap easily-removable/deprecatable solution that can help solving the issue right now? Regarding the Basically what I'm trying to ask here is, are the benefits of implementing such a change outweighed by the cons? |
8597049
to
3c8bf07
Compare
platform
object for local development
3f33359
to
d772c8f
Compare
d772c8f
to
d8e6a5a
Compare
@@ -33,6 +33,7 @@ | |||
}, | |||
"dependencies": { | |||
"@cloudflare/workers-types": "^4.20231121.0", | |||
"wrangler": "^3.24.0", |
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.
Will users expect to install wrangler
and control what version of wrangler
is used? I'm wondering if it's better here or in peerDependencies
. You probably know better than I do though
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.
Yes, when developing their app they will use wrangler
anyways so I think that it makes sense to have it as a peer dependency, I've moved it 👍
But with this change, since wrangler
might actually not be installed, I guess I might need to change the wrangler import for getBindingsProxy
to be dynamic and try-catched so that we can check if it actually succeeds
What do you think?
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.
Yes, putting it in a try-catch so we can print a nice error message when it is not present makes sense to me
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.
Can you regenerate the pnpm lock file as well?
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 did re-run pnpm i
but it didn't generate any different in the pnpm lock file, I guess it should? 😕
A couple of the maintainers suggested making the API |
* This function is used to allow the adapter to emulate the platform object during dev and | ||
* preview | ||
*/ | ||
emulatePlatform?(svelteKitConfig: KitConfig): MaybePromise<App.Platform>; |
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 don't think we need to pass the config into here, unless you had something specific in mind? If we do need it, it should be opts: { config: KitConfig }
so that we can add other stuff in future if necessary
emulatePlatform?(svelteKitConfig: KitConfig): MaybePromise<App.Platform>; | |
emulatePlatform?(): MaybePromise<App.Platform>; |
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.
It was just for future proofing the solution/allowing other adapters to add things on top of it (which your solution addresses better)
Anyways it is not needed and it can be omitted entirely if you prefer (and/or omitted for now and potentially added when/if needed)
Thank you for taking this on, really excited for the possibilities this opens up. API-wise, @benmccann mentioned doing I would actually go further and suggest that we do this: async emulate() {
// called once at the beginning of `dev` and `preview`. `getBindingsProxy()` would go here,
// rather than the top of the module, so it's not called during `build`
return {
async platform({ config }) => {
// config here is the resolved route config, _not_ the contents of `svelte.config.js`
}
};
} Another fun wrinkle is that we probably shouldn't populate |
Thinking about what the implementation will look like for some of that stuff, it's probably quite finicky and involves a lot of knowledge of the internals, so if you're cool with it @dario-piotrowicz then maybe it's more logical for us to pick it up from here? Appreciate all the work that you've put into it so far! |
I've made a start in #11730, though there are a few TODOs and open questions still outstanding |
@Rich-Harris Thanks so much for looking into this 🙂 We on the Cloudflare team are really keen and hopeful to get this sort of dev bindings support ready as soon as we can. We're also working with other framework authors to apply the same across the whole js ecosystem. So yeah please feel free to take it over if you want, but I am always here to pick it back up and/or help in any manner I can. |
Thank you @dario-piotrowicz! If you have any pointers on how to populate |
Probably a useless comment here that you are all already aware of, but wrangler-proxy is a great example of how to use wrangler in development for SK - but I thought it might help 🤷♂️ |
This PR introduces an
emulatePlatform
method that adapters can provide that allows them to emulate theplatform
object for users during local development (and preview).More specifically this is used in the Cloudflare adapters, so that (using the newly wrangler utility
getBindingsProxy
) they read awrangler.toml
file from the user's directory and generate an appropriate platform object for it accordingly.As an example of the use of this, see this simple demo I made.
This PR has significantly changed from the original idea/proposal I had, just for recording it here's the original PR description:
Original `devPlatform` idea (outdated)
Hello 👋The changes here introduce a
devPlatform
option for thekit
config so that developers can provide their own platform mocking implementation for local development.Given the conversation in #4292, @Rich-Harris' pushback), #3535, #8397, and so on... I am aware that the changes here might just get rejected but I wanted to propose them anyways as I think that this could be a valid short/medium term solution that many developers could take advantage of right now (instead of making them indefinitely wait for a better solution that might or might not come at some point in the future).
From an outsider's perspective and the issues I mentioned above (which seem pretty stale) I don't know what the Svelte team's current plan is regarding this issue, if you have a different/better solution in the works that can be applied soon-ish that'd be awesome, but if you don't please give this PR's a second thought since this clearly seems to be an issue that various developers struggle with. Moreover please note that the changes here are pretty minimal so they should be very simple to deprecate/remove in the future if a better alternative does get implemented.
As an example of the use of this
devPlatform
config, I've implemented this simple demo which shows how this could be used to locally develop an application using a Cloudflare KV binding, you can see here how thedevPlatform
is being set and here how it is being used.Currently the only alternative workaround that can allow developers to use bindings with the Vite dev server is by leveraging server hooks, I included an example of that here. As you can see this is much much more cumbersome than the proposed API and also effects the production build, so it's definitely not a great alternative in my opinion.
Additionally I'd like to point out that there's a precedence of such an API, QwikCity is actually doing the exact same thing in their Vite plugin (see their docs), showing that even if not perfect this seems to be an acceptable solution for this problem.
resolves #4292
(also #4908, and maybe more)
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:
.