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

(WIP) introduce a way for users to emulate the platform object for local development #11323

Closed
wants to merge 5 commits into from

Conversation

dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Dec 14, 2023

This PR introduces an emulatePlatform method that adapters can provide that allows them to emulate the platform 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 a wrangler.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 the kit 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 the devPlatform 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:

  • 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:.

Copy link

changeset-bot bot commented Dec 14, 2023

⚠️ No Changeset found

Latest commit: 6781d07

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@saturnonearth
Copy link

This would be great

Copy link
Member

@benmccann benmccann left a 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

@Conduitry
Copy link
Member

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 handle hook that does some assignments in an if (dev) { ... }, as mentioned in the PR description. This gets treeshaken out of production builds. It seems like an entirely acceptable workaround to me.

@dario-piotrowicz
Copy link
Contributor Author

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

@benmccann are saying that you'd be ok with this approach if I moved the devPlatform to be part of an adaptor config instead? or are you saying that this feels like the wrong approach/API full stop?

@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented Dec 16, 2023

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 handle hook that does some assignments in an if (dev) { ... }, as mentioned in the PR description. This gets treeshaken out of production builds. It seems like an entirely acceptable workaround to me.

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 handle solution, again I kind of do agree with you that it is a kind of acceptable but it does still require developers to implement this hands-on hack which does have (even if minimal) implications in the production build (since the dev code can indeed get tree shaken away but the handle itself cannot).

Basically what I'm trying to ask here is, are the benefits of implementing such a change outweighed by the cons?

@dario-piotrowicz dario-piotrowicz marked this pull request as draft January 16, 2024 23:23
@eltigerchino eltigerchino added adapters - general Support for functionality general to all adapters feature / enhancement New feature or request and removed adapters - general Support for functionality general to all adapters labels Jan 17, 2024
@dario-piotrowicz dario-piotrowicz changed the title introduce devPlatform kit configuration (WIP) introduce a way for users to emulate the platform object for local development Jan 20, 2024
@dario-piotrowicz dario-piotrowicz force-pushed the devPlatform branch 2 times, most recently from 3f33359 to d772c8f Compare January 20, 2024 16:20
@@ -33,6 +33,7 @@
},
"dependencies": {
"@cloudflare/workers-types": "^4.20231121.0",
"wrangler": "^3.24.0",
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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?

Copy link
Contributor Author

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? 😕

@benmccann
Copy link
Member

A couple of the maintainers suggested making the API emulate: ()=>({platform}) in order to future proof it. That way if we ever need to provide arguments to create the platform we won't have to introduce a breaking change to do so. It's a little less pretty in the short term, but since it's usage is hidden away from users inside the adapter I think that's okay.

* This function is used to allow the adapter to emulate the platform object during dev and
* preview
*/
emulatePlatform?(svelteKitConfig: KitConfig): MaybePromise<App.Platform>;
Copy link
Member

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

Suggested change
emulatePlatform?(svelteKitConfig: KitConfig): MaybePromise<App.Platform>;
emulatePlatform?(): MaybePromise<App.Platform>;

Copy link
Contributor Author

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)

@Rich-Harris
Copy link
Member

Thank you for taking this on, really excited for the possibilities this opens up.

API-wise, @benmccann mentioned doing emulate: ()=>({platform}) instead, and the real reason for that is that we might want to add other things besides platform for introducing new globals, banning certain imports (e.g. unsupported node:* modules), and so on.

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 platform for routes that are prerender: true (or prerender: 'auto') during development, unless we wanted to also populate it during prerendering (which I don't think we do).

@Rich-Harris
Copy link
Member

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!

@Rich-Harris
Copy link
Member

I've made a start in #11730, though there are a few TODOs and open questions still outstanding

@dario-piotrowicz
Copy link
Contributor Author

@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.
Whatever you think it's best, both for your framework and for hopefully expedite this feature's implementation works for us, and we're always happy to keep working on this and/or support in any other way we can.

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.

@dario-piotrowicz dario-piotrowicz deleted the devPlatform branch January 24, 2024 16:09
@Rich-Harris
Copy link
Member

Thank you @dario-piotrowicz! If you have any pointers on how to populate caches and context, and how to test it locally (see notes on the other PR), we'd be very grateful. Also, is it okay that we're not calling bindings.dispose but are just killing the process?

@saturnonearth
Copy link

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 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters - general Support for functionality general to all adapters feature / enhancement New feature or request pkg:adapter-cloudflare pkg:adapter-cloudflare-workers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Platform context fallbacks
6 participants