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.

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

DRY out data suffix handling #7419

Merged
merged 3 commits into from
Oct 28, 2022
Merged

DRY out data suffix handling #7419

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
fe7be51
centralise data suffix logic
Rich-Harris Oct 28, 2022
367551a
Merge branch 'master' into data-suffix
Rich-Harris Oct 28, 2022
1fb72d4
apply to cookie logic
Rich-Harris Oct 28, 2022
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
2 changes: 0 additions & 2 deletions packages/kit/src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,3 @@
export const SVELTE_KIT_ASSETS = '/_svelte_kit_assets';

export const GENERATED_COMMENT = '// this file is generated — do not edit it\n';

export const DATA_SUFFIX = '/__data.json';
5 changes: 2 additions & 3 deletions packages/kit/src/runtime/client/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { onMount, tick } from 'svelte';
import { make_trackable, decode_params, normalize_path } from '../../utils/url.js';
import { make_trackable, decode_params, normalize_path, add_data_suffix } from '../../utils/url.js';
import { find_anchor, get_base_uri, scroll_state } from './utils.js';
import {
lock_fetch,
Expand All @@ -14,7 +14,6 @@ import Root from '__GENERATED__/root.svelte';
import { nodes, server_loads, dictionary, matchers, hooks } from '__GENERATED__/client-manifest.js';
import { HttpError, Redirect } from '../control.js';
import { stores } from './singletons.js';
import { DATA_SUFFIX } from '../../constants.js';
import { unwrap_promises } from '../../utils/promises.js';
import * as devalue from 'devalue';

Expand Down Expand Up @@ -1511,7 +1510,7 @@ export function create_client({ target, base, trailing_slash }) {
*/
async function load_data(url, invalid) {
const data_url = new URL(url);
data_url.pathname = url.pathname.replace(/\/$/, '') + DATA_SUFFIX;
data_url.pathname = add_data_suffix(url.pathname);

const res = await native_fetch(data_url.href, {
headers: {
Expand Down
7 changes: 2 additions & 5 deletions packages/kit/src/runtime/server/cookie.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { parse, serialize } from 'cookie';
import { DATA_SUFFIX } from '../../constants.js';
import { normalize_path } from '../../utils/url.js';
import { has_data_suffix, normalize_path, strip_data_suffix } from '../../utils/url.js';

/**
* Tracks all cookies set during dev mode so we can emit warnings
Expand Down Expand Up @@ -76,9 +75,7 @@ export function get_cookies(request, url, options) {
const normalized = normalize_path(
// Remove suffix: 'foo/__data.json' would mean the cookie path is '/foo',
// whereas a direct hit of /foo would mean the cookie path is '/'
url.pathname.endsWith(DATA_SUFFIX)
? url.pathname.slice(0, -DATA_SUFFIX.length)
: url.pathname,
has_data_suffix(url.pathname) ? strip_data_suffix(url.pathname) : url.pathname,
options.trailing_slash
);
// Emulate browser-behavior: if the cookie is set at '/foo/bar', its path is '/foo'
Expand Down
8 changes: 2 additions & 6 deletions packages/kit/src/runtime/server/data/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ import { normalize_error } from '../../../utils/error.js';
import { once } from '../../../utils/functions.js';
import { load_server_data } from '../page/load_data.js';
import { data_response, handle_error_and_jsonify } from '../utils.js';
import { normalize_path } from '../../../utils/url.js';
import { DATA_SUFFIX } from '../../../constants.js';
import { normalize_path, strip_data_suffix } from '../../../utils/url.js';

/**
* @param {import('types').RequestEvent} event
Expand All @@ -31,10 +30,7 @@ export async function render_data(event, route, options, state) {
let aborted = false;

const url = new URL(event.url);
url.pathname = normalize_path(
url.pathname.slice(0, -DATA_SUFFIX.length),
options.trailing_slash
);
url.pathname = normalize_path(strip_data_suffix(url.pathname), options.trailing_slash);

const new_event = { ...event, url };

Expand Down
13 changes: 9 additions & 4 deletions packages/kit/src/runtime/server/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,15 @@ import { respond_with_error } from './page/respond_with_error.js';
import { coalesce_to_error } from '../../utils/error.js';
import { is_form_content_type } from '../../utils/http.js';
import { GENERIC_ERROR, handle_fatal_error } from './utils.js';
import { decode_params, disable_search, normalize_path } from '../../utils/url.js';
import {
decode_params,
disable_search,
has_data_suffix,
normalize_path,
strip_data_suffix
} from '../../utils/url.js';
import { exec } from '../../utils/routing.js';
import { render_data } from './data/index.js';
import { DATA_SUFFIX } from '../../constants.js';
import { add_cookies_to_headers, get_cookies } from './cookie.js';
import { HttpError } from '../control.js';
import { create_fetch } from './fetch.js';
Expand Down Expand Up @@ -57,8 +62,8 @@ export async function respond(request, options, state) {
decoded = decoded.slice(options.paths.base.length) || '/';
}

const is_data_request = decoded.endsWith(DATA_SUFFIX);
if (is_data_request) decoded = decoded.slice(0, -DATA_SUFFIX.length) || '/';
const is_data_request = has_data_suffix(decoded);
if (is_data_request) decoded = strip_data_suffix(decoded);

if (!state.prerendering?.fallback) {
const matchers = await options.manifest._.matchers();
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/src/runtime/server/page/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as devalue from 'devalue';
import { DATA_SUFFIX } from '../../../constants.js';
import { compact } from '../../../utils/array.js';
import { normalize_error } from '../../../utils/error.js';
import { add_data_suffix } from '../../../utils/url.js';
import { HttpError, Redirect } from '../../control.js';
import {
get_option,
Expand Down Expand Up @@ -76,7 +76,7 @@ export async function render_page(event, route, page, options, state, resolve_op
}

const should_prerender_data = nodes.some((node) => node?.server);
const data_pathname = event.url.pathname.replace(/\/$/, '') + DATA_SUFFIX;
const data_pathname = add_data_suffix(event.url.pathname);

// it's crucial that we do this before returning the non-SSR response, otherwise
// SvelteKit will erroneously believe that the path has been prerendered,
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/src/runtime/server/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as devalue from 'devalue';
import { DATA_SUFFIX } from '../../constants.js';
import { negotiate } from '../../utils/http.js';
import { has_data_suffix } from '../../utils/url.js';
import { HttpError } from '../control.js';

/** @param {any} body */
Expand Down Expand Up @@ -144,7 +144,7 @@ export function handle_fatal_error(event, options, error) {
'text/html'
]);

if (event.url.pathname.endsWith(DATA_SUFFIX) || type === 'application/json') {
if (has_data_suffix(event.url.pathname) || type === 'application/json') {
return new Response(JSON.stringify(body), {
status,
headers: { 'content-type': 'application/json; charset=utf-8' }
Expand Down
17 changes: 17 additions & 0 deletions packages/kit/src/utils/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,20 @@ export function disable_search(url) {
});
}
}

const DATA_SUFFIX = '/__data.json';

/** @param {string} pathname */
export function has_data_suffix(pathname) {
return pathname.endsWith(DATA_SUFFIX);
}

/** @param {string} pathname */
export function add_data_suffix(pathname) {
return pathname.replace(/\/$/, '') + DATA_SUFFIX;
}

/** @param {string} pathname */
export function strip_data_suffix(pathname) {
return pathname.slice(0, -DATA_SUFFIX.length);
Copy link
Member

Choose a reason for hiding this comment

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

should we use has_data_suffix here and make this a noop in case it's not the suffix? Would reduce code at one call site.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a huge fan of functions that are 'forgiving' about their inputs — it adds modest convenience (albeit at a single call site in our case, IOW it's just moving the logic around rather than reducing repetition) at the cost of greater uncertainty and wasted computation (however negligible)

}