Skip to content

Conversation

Rich-Harris
Copy link
Member

WIP — there's a few places where we're getting data in load functions via a server route. This is a hangover from the days before server load functions, and is entirely unnecessary

Copy link

vercel bot commented Oct 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
omnisite ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2024 2:14pm

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is a good approach. This will will mean that on every invocation of the runtime we'll read all the files over and over again - I'm not sure if they are even part of the build output since they live in content and that likely isn't copied over.

});
}

if (dev && !client) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep this - if we open this repo up and people want to work on this locally, they will have no way of interact with saved REPL, we for sure don't want to give out credentials to everyone

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they can't load specific arbitrary REPLs but so what?

Copy link
Member

@Conduitry Conduitry Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was something I added in sveltejs/sites#281, and I found it very useful for running the REPL locally with a modified version of the compiler to see whether a given change actually fixed someone's repro. It would be a shame to lose it.

If that means doing something hacky where the server load function needs to proxy to the corresponding internal endpoint on the real server and then decode that response so it can be re-encoded, so be it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you just need the file contents, right? And you can easily get that from the hash now — just make a change to the repro in question (add a space then delete it, whatever) then copy the hash from the URL bar, and you can run the same thing locally

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's true that the hash-based REPL storage could also address this.

Previously, there were a couple of extra hurdles. The "file contents" could be plural files contents (each of which would have to be copied separately), but also refreshing the local REPL (to get another new version of the compiler as part of the feedback cycle) would result in the just-pasted REPL being lost and having to be re-pasted (including potentially re-generating multiple files, if the repro had them).

However if one can easily generate hash-based REPL URLs from DB ID-based REPL URLs, that does alleviate a lot of these problems, at least for repros that don't require features that are only available in DB ID-based REPLs.

In that vein, does it make sense to make the REPL enable those more sensitive features (and the skip the 'click to run' button) for hash-based REPLs when running in development mode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, very possibly (though we want to avoid dev/prod discrepancies where we can of course). I think we could file that under problems-for-future-us though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean ... if it's just about leaving this code in, why not just undo the deletion and not have to worry about "enable/disable certain restrictions in certain places"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it requires us to expose an unnecessary endpoint

@Rich-Harris
Copy link
Member Author

What if we put it behind a load_examples function? (I had it working that way at one point but opted for examples_promise instead, but it's easy enough to just expose load_examples and memoize it)

@dummdidumm
Copy link
Member

memoizing it won't help much in a serverless world. But yes, it would at least restrict it to only be loaded on playground routes.

Is there a different angle we can take? Rather than generating a server route, we're generating some file during build/dev that is imported? AFAIK this is the way it works on the current svelte.dev site.

@Rich-Harris
Copy link
Member Author

memoizing it won't help much in a serverless world

True. But the real question is whether the deployment making an HTTP request for a prerendered JSON file is going to be faster than generating the same data on the machine where the request is being handled, and where the files already live. Seems unlikely, honestly

@Rich-Harris Rich-Harris mentioned this pull request Oct 10, 2024
@dummdidumm
Copy link
Member

dummdidumm commented Oct 10, 2024

Not sure I follow, SvelteKit recognizes fetches to itself so there's no network overhead.

Update: scratch that I think it falls back to requesting from the network, maybe SvelteKit could get smarter here and handle this without a network jump as well (although this is somewhat of an edge case)

@Rich-Harris
Copy link
Member Author

SvelteKit does need to get smarter so that we can combine prerendered layouts with dynamic pages and vice versa, but that's likely a few months off

@Rich-Harris
Copy link
Member Author

closing as stale, we can revisit when we eventually implement PPR etc

@dummdidumm dummdidumm deleted the reduce-json-indirection branch October 30, 2024 18:14
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.

3 participants