Skip to content

get rid of unnecessary .json endpoints #323

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

get rid of unnecessary .json endpoints #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
164847d
get rid of unnecessary examples/all.json endpoint
Rich-Harris Oct 10, 2024
b351ace
load once
Rich-Harris Oct 10, 2024
09b4ca3
drop non-working-credentials workaround, we should just set everyone …
Rich-Harris Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion apps/svelte.dev/src/lib/server/content.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,49 @@ function create_docs() {

export const docs = create_docs();

export const examples = index.examples.children;
export const examples_promise: ReturnType<typeof load_examples> = new Promise(async (fulfil) =>
fulfil(await load_examples())
);

async function load_examples() {
const sections = [];

for (const section of index.examples.children) {
const examples = [];

for (const document of section.children) {
// TODO 'components' is a misnomer, this can include other files
const components = [];

for (const [file, source] of Object.entries(document.assets!)) {
const dot = file.lastIndexOf('.');
let name = file.slice(0, dot);
let type = file.slice(dot + 1);

components.push({ name, type, source: await read(source).text() });
}

components.sort((a, b) => {
if (a.name === 'App' && a.type === 'svelte') return -1;
if (b.name === 'App' && b.type === 'svelte') return 1;

if (a.type !== b.type) return a.type === 'svelte' ? -1 : 1;

return a.name < b.name ? -1 : 1;
});

examples.push({
title: document.metadata.title,
slug: document.slug.split('/').pop()!,
components
});
}

sections.push({
title: section.metadata.title,
examples
});
}

return sections;
}
9 changes: 6 additions & 3 deletions apps/svelte.dev/src/routes/(authed)/playground/[id]/+page.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
import { error } from '@sveltejs/kit';
import type { Examples } from '../api/examples/all.json/+server.js';
import { examples_promise } from '$lib/server/content.js';

const examples = await examples_promise;

export async function load({ fetch, params, url }) {
const examples_res = fetch('/playground/api/examples/all.json').then((r) => r.json());
// TODO skip the .json indirection
const res = await fetch(`/playground/api/${params.id}.json`);

if (!res.ok) {
error(res.status);
}

const [gist, examples] = await Promise.all([res.json(), examples_res as Promise<Examples>]);
const gist = await res.json();

return {
gist,
// TODO do this work in layout instead
examples: examples
.filter((section) => !section.title.includes('Embeds'))
.map((section) => ({
Expand Down
23 changes: 4 additions & 19 deletions apps/svelte.dev/src/routes/(authed)/playground/api/[id].json/+server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import { dev } from '$app/environment';
import { client } from '$lib/db/client.js';
import * as gist from '$lib/db/gist.js';
import { examples } from '$lib/server/content';
import { index, examples_promise } from '$lib/server/content';
import { error, json } from '@sveltejs/kit';
import type { Examples } from '../examples/all.json/+server.js';

export const prerender = 'auto';

const UUID_REGEX = /^[0-9a-f]{8}-?[0-9a-f]{4}-?[0-9a-f]{4}-?[0-9a-f]{4}-?[0-9a-f]{12}$/;

const examples = await examples_promise;

export async function GET({ fetch, params }) {
const examples: Examples = await fetch('/playground/api/examples/all.json').then((r) => r.json());
const example = examples
.flatMap((section) => section.examples)
.find((example) => example.slug.split('/').pop() === params.id);
Expand All @@ -25,21 +25,6 @@ export async function GET({ fetch, params }) {
});
}

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 •
edited
Loading

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

// in dev with no local Supabase configured, proxy to production
// this lets us at least load saved REPLs
const res = await fetch(`https://svelte.dev/playground/api/${params.id}.json`);

// returning the response directly results in a bizarre
// content encoding error, so we create a new one
return new Response(await res.text(), {
status: res.status,
headers: {
'content-type': 'application/json'
}
});
}

if (!UUID_REGEX.test(params.id)) {
error(404);
}
Expand All @@ -62,7 +47,7 @@ export async function GET({ fetch, params }) {
}

export async function entries() {
return examples
return index.examples.children
.flatMap((section) => section.children)
.map((example) => ({ id: example.slug.split('/').pop()! }));
}
59 changes: 0 additions & 59 deletions apps/svelte.dev/src/routes/(authed)/playground/api/examples/all.json/+server.ts
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions apps/svelte.dev/src/routes/content.json/+server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { index, docs as _docs, examples } from '$lib/server/content';
import { index, docs as _docs } from '$lib/server/content';
import { json } from '@sveltejs/kit';
import { transform, slugify, clean } from '@sveltejs/site-kit/markdown';
import type { Block } from '@sveltejs/site-kit/search';
Expand Down Expand Up @@ -82,7 +82,7 @@ async function content() {
}
}

for (const section of examples) {
for (const section of index.examples.children) {
for (const example of section.children) {
blocks.push({
breadcrumbs: ['Examples', section.metadata.title, example.metadata.title],
Expand Down
Loading