Skip to content

chore: cleanup promise handling #12284

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 16 commits into from
Closed

chore: cleanup promise handling #12284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c00b1f3
chore: cleanup promise handling
benmccann May 31, 2024
0d23637
revert breaking changes
Rich-Harris Jun 3, 2024
88eeab6
revert
Rich-Harris Jun 3, 2024
6f5b2d8
fix
Rich-Harris Jun 3, 2024
c4c2175
Update packages/kit/src/runtime/client/fetcher.js
Rich-Harris Jun 3, 2024
5512a5a
appease the nanny
Rich-Harris Jun 3, 2024
13ee5f8
Merge branch 'promise-cleanup' of github.com:sveltejs/kit into promis…
Rich-Harris Jun 3, 2024
8874d96
another
Rich-Harris Jun 3, 2024
336b639
remove need for an ignore
benmccann Jun 4, 2024
743c640
address some no-floating-promises issues
benmccann Jun 4, 2024
24fc701
format
benmccann Jun 4, 2024
f156eaa
load in parallel
benmccann Jun 4, 2024
e244c28
enqueue in parallel
benmccann Jun 4, 2024
e075503
format
benmccann Jun 4, 2024
f9e02f4
fix stupid mistake
benmccann Jun 4, 2024
dd553bc
revert check_stack_trace change
benmccann Jun 4, 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
22 changes: 21 additions & 1 deletion eslint.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,17 @@ import svelte_config from '@sveltejs/eslint-config';
export default [
...svelte_config,
{
languageOptions: {
parserOptions: {
project: true
}
},
rules: {
'@typescript-eslint/await-thenable': 'error',
// '@typescript-eslint/no-floating-promises': 'error',
// '@typescript-eslint/no-misused-promises': 'error',
Copy link
Member

Choose a reason for hiding this comment

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

I took a look at the errors that arise if this rule is enabled, and not a single one is valid. It's just ESLint whining pointlessly

Suggested change
// '@typescript-eslint/no-misused-promises': 'error',

Copy link
Member

Choose a reason for hiding this comment

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

a lot more errors pop up with no-floating-promises and i haven't looked at them all yet, but of the ones i have looked at it's the same story. definitely not something we want

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 don't understand why the issues no-floating-promises is identifying shouldn't be addressed. I just pushed a commit addressing a lot of them: 743c640. We can drop it if they're really not issues. This isn't my strong point, so it's quite possible I'm missing something. I figured I'd push it though so that I can at least learn what that is as it's easier to discuss that way.

Copy link
Member

Choose a reason for hiding this comment

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

They're really not issues, but the rule tricked you into breaking code that works perfectly well

'@typescript-eslint/no-unused-expressions': 'off',
'@typescript-eslint/require-await': 'error',
'no-undef': 'off'
}
},
Expand All @@ -15,7 +24,18 @@ export default [
'packages/adapter-static/test/apps/*/build',
'packages/adapter-cloudflare/files',
'packages/adapter-netlify/files',
'packages/adapter-node/files'
'packages/adapter-node/files',
'packages/adapter-node/rollup.config.js',
'packages/adapter-node/tests/smoke.spec.js',
'packages/adapter-static/test/apps',
'packages/create-svelte/shared',
'packages/create-svelte/templates',
'packages/kit/src/core/sync/create_manifest_data/test/samples',
'packages/kit/test/apps',
'packages/kit/test/build-errors',
'packages/kit/test/prerendering',
'packages/package/test/errors',
'packages/package/test/fixtures'
]
}
];
4 changes: 2 additions & 2 deletions packages/adapter-auto/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ function detect_package_manager() {
}

/** @param {string} name */
async function import_from_cwd(name) {
function import_from_cwd(name) {
const cwd = pathToFileURL(process.cwd()).href;
const url = await resolve(name, cwd + '/x.js');
const url = resolve(name, cwd + '/x.js');

return import(url);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/adapter-netlify/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export default function ({ split = false, edge = edge_set_in_env_var } = {}) {

await generate_edge_functions({ builder });
} else {
await generate_lambda_functions({ builder, split, publish });
generate_lambda_functions({ builder, split, publish });
}
},

Expand Down Expand Up @@ -182,7 +182,7 @@ async function generate_edge_functions({ builder }) {
* @param { string } params.publish
* @param { boolean } params.split
*/
async function generate_lambda_functions({ builder, publish, split }) {
function generate_lambda_functions({ builder, publish, split }) {
builder.mkdirp('.netlify/functions-internal/.svelte-kit');

/** @type {string[]} */
Expand Down
2 changes: 1 addition & 1 deletion packages/adapter-node/src/handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ const ssr = async (req, res) => {
return;
}

setResponse(
await setResponse(
res,
await server.respond(request, {
platform: { req },
Expand Down
2 changes: 1 addition & 1 deletion packages/adapter-vercel/files/serverless.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export default async (req, res) => {

const request = await getRequest({ base: `https://${req.headers.host}`, request: req });

setResponse(
await setResponse(
res,
await server.respond(request, {
getClientAddress() {
Expand Down
1 change: 1 addition & 0 deletions packages/create-svelte/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import path from 'node:path';
import { mkdirp, copy, dist } from './utils.js';

/** @type {import('./types/index.js').create} */
// eslint-disable-next-line @typescript-eslint/require-await
export async function create(cwd, options) {
mkdirp(cwd);

Expand Down
2 changes: 1 addition & 1 deletion packages/create-svelte/scripts/build-templates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,4 +305,4 @@ async function main() {
await generate_templates(shared);
}

main();
await main();
2 changes: 1 addition & 1 deletion packages/create-svelte/test/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ for (const template of templates) {
const cwd = path.join(test_workspace_dir, `${template}-${types}`);
fs.rmSync(cwd, { recursive: true, force: true });

create(cwd, {
await create(cwd, {
name: `create-svelte-test-${template}-${types}`,
template,
types,
Expand Down
5 changes: 3 additions & 2 deletions packages/enhanced-img/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ import { image } from './preprocessor.js';
/**
* @returns {Promise<import('vite').Plugin[]>}
*/
// eslint-disable-next-line @typescript-eslint/require-await
export async function enhancedImages() {
const imagetools_instance = await imagetools_plugin();
const imagetools_instance = imagetools_plugin();
return !process.versions.webcontainer
? [image_plugin(imagetools_instance), imagetools_instance]
: [];
Expand Down Expand Up @@ -60,7 +61,7 @@ const fallback = {
'.webp': 'png'
};

async function imagetools_plugin() {
function imagetools_plugin() {
/** @type {Partial<import('vite-imagetools').VitePluginOptions>} */
const imagetools_opts = {
defaultDirectives: async ({ pathname, searchParams: qs }, metadata) => {
Expand Down
2 changes: 1 addition & 1 deletion packages/enhanced-img/test/preprocessor.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ it('Image preprocess snapshot test', async () => {
// Make imports readable
const ouput = processed.code.replace(/import/g, '\n\timport');

expect(ouput).toMatchFileSnapshot('./Output.svelte');
await expect(ouput).toMatchFileSnapshot('./Output.svelte');
});

it('parses a minimized object', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/postinstall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ try {

try {
const config = await load_config();
await sync.all(config, 'development');
sync.all(config, 'development');
} catch (error) {
console.error('Error while trying to sync SvelteKit config');
console.error(error);
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/src/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ prog
try {
const config = await load_config();
const sync = await import('./core/sync/sync.js');
await sync.all_types(config, mode);
sync.all_types(config, mode);
} catch (error) {
handle_error(error);
}
Expand Down
18 changes: 10 additions & 8 deletions packages/kit/src/core/postbuild/prerender.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ async function prerender({ out, manifest_path, metadata, verbose, env }) {

const body = Buffer.from(await response.arrayBuffer());

save('pages', response, body, decoded, encoded, referrer, 'linked');
await save('pages', response, body, decoded, encoded, referrer, 'linked');

for (const [dependency_path, result] of dependencies) {
// this seems circuitous, but using new URL allows us to not care
Expand All @@ -257,7 +257,7 @@ async function prerender({ out, manifest_path, metadata, verbose, env }) {

const body = result.body ?? new Uint8Array(await result.response.arrayBuffer());

save(
await save(
'dependencies',
result.response,
body,
Expand Down Expand Up @@ -305,7 +305,7 @@ async function prerender({ out, manifest_path, metadata, verbose, env }) {
/** @type {Set<string>} */ (expected_hashlinks.get(key)).add(decoded);
}

enqueue(decoded, decode_uri(pathname), pathname);
await enqueue(decoded, decode_uri(pathname), pathname);
}
}
}
Expand All @@ -319,7 +319,7 @@ async function prerender({ out, manifest_path, metadata, verbose, env }) {
* @param {string | null} referrer
* @param {'linked' | 'fetched'} referenceType
*/
function save(category, response, body, decoded, encoded, referrer, referenceType) {
async function save(category, response, body, decoded, encoded, referrer, referenceType) {
const response_type = Math.floor(response.status / 100);
const headers = Object.fromEntries(response.headers);

Expand All @@ -341,7 +341,7 @@ async function prerender({ out, manifest_path, metadata, verbose, env }) {
if (location) {
const resolved = resolve(encoded, location);
if (is_root_relative(resolved)) {
enqueue(decoded, decode_uri(resolved), resolved);
await enqueue(decoded, decode_uri(resolved), resolved);
Copy link
Member

Choose a reason for hiding this comment

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

why are we awaiting these all? I imagine this adding up when prerendering a lot of files, hurting perf in the end, and I can't really imagine there being errors that are otherwise uncaught.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's such a quick action I didn't really imagine it adding up. I've updated it to enqueue them in parallel instead though to be safe

Copy link
Member

Choose a reason for hiding this comment

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

See, stuff like this is exactly why these rules are so dangerous — I'm pretty sure this creates deadlock, and I'm pretty sure that's why the tests started failing with the commit that added this.

You have to treat ESLint like an idiot child. You can give it very simple tasks from time to time, but don't trust it with anything important.

Let's revert all this

}

if (!headers['x-sveltekit-normalize']) {
Expand Down Expand Up @@ -449,6 +449,7 @@ async function prerender({ out, manifest_path, metadata, verbose, env }) {
read: (file) => createReadableStream(`${config.outDir}/output/server/${file}`)
});

const entries_to_enqueue = [];
for (const entry of config.prerender.entries) {
if (entry === '*') {
for (const [id, prerender] of prerender_map) {
Expand All @@ -459,19 +460,20 @@ async function prerender({ out, manifest_path, metadata, verbose, env }) {

if (processed_id.includes('[')) continue;
const path = `/${get_route_segments(processed_id).join('/')}`;
enqueue(null, config.paths.base + path);
entries_to_enqueue.push({ path: config.paths.base + path });
}
}
} else {
enqueue(null, config.paths.base + entry);
entries_to_enqueue.push({ path: config.paths.base + entry });
}
}

for (const { id, entries } of route_level_entries) {
for (const entry of entries) {
enqueue(null, config.paths.base + entry, undefined, id);
entries_to_enqueue.push({ path: config.paths.base + entry, id });
}
}
await Promise.all(entries_to_enqueue.map(({ path, id }) => enqueue(null, path, undefined, id)));
Copy link
Member

Choose a reason for hiding this comment

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

why is this better than what existed before this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's clearer to me as it's explicit that we intend to enqueue them in parallel. it also handles rejected promise errors which would not be handled before. while it's a pretty simple method that's unlikely to fail, but everytime I come across something like that in a code review I have to review it and assess the risk that it might fail and it's easier for me to not have to do that

Copy link
Member

Choose a reason for hiding this comment

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

Rejected promise errors are handled already. That's what the await q.done() two lines later is for. This is duplicative and less efficient, it adds nothing!


await q.done();

Expand Down
16 changes: 8 additions & 8 deletions packages/kit/src/core/sync/sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ export function init(config, mode) {
* Update SvelteKit's generated files
* @param {import('types').ValidatedConfig} config
*/
export async function create(config) {
export function create(config) {
const manifest_data = create_manifest_data({ config });

const output = path.join(config.kit.outDir, 'generated');

write_client_manifest(config.kit, manifest_data, `${output}/client`);
write_server(config, output);
write_root(manifest_data, output);
await write_all_types(config, manifest_data);
write_all_types(config, manifest_data);

return { manifest_data };
}
Expand All @@ -44,8 +44,8 @@ export async function create(config) {
* @param {import('types').ManifestData} manifest_data
* @param {string} file
*/
export async function update(config, manifest_data, file) {
await write_types(config, manifest_data, file);
export function update(config, manifest_data, file) {
write_types(config, manifest_data, file);

return { manifest_data };
}
Expand All @@ -55,20 +55,20 @@ export async function update(config, manifest_data, file) {
* @param {import('types').ValidatedConfig} config
* @param {string} mode The Vite mode
*/
export async function all(config, mode) {
export function all(config, mode) {
init(config, mode);
return await create(config);
return create(config);
}

/**
* Run sync.init and then generate all type files.
* @param {import('types').ValidatedConfig} config
* @param {string} mode The Vite mode
*/
export async function all_types(config, mode) {
export function all_types(config, mode) {
init(config, mode);
const manifest_data = create_manifest_data({ config });
await write_all_types(config, manifest_data);
write_all_types(config, manifest_data);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/src/core/sync/write_types/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const cwd = process.cwd();
* @param {import('types').ValidatedConfig} config
* @param {import('types').ManifestData} manifest_data
*/
export async function write_all_types(config, manifest_data) {
export function write_all_types(config, manifest_data) {
if (!ts) return;

const types_dir = `${config.kit.outDir}/types`;
Expand Down Expand Up @@ -133,7 +133,7 @@ export async function write_all_types(config, manifest_data) {
* @param {import('types').ManifestData} manifest_data
* @param {string} file
*/
export async function write_types(config, manifest_data, file) {
export function write_types(config, manifest_data, file) {
if (!ts) return;

if (!path.basename(file).startsWith('+')) {
Expand Down
24 changes: 12 additions & 12 deletions packages/kit/src/core/sync/write_types/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const cwd = fileURLToPath(new URL('./test', import.meta.url));
/**
* @param {string} dir
*/
async function run_test(dir) {
function run_test(dir) {
rimraf(path.join(cwd, dir, '.svelte-kit'));

const initial = options({}, 'config');
Expand All @@ -25,22 +25,22 @@ async function run_test(dir) {
const manifest = create_manifest_data({
config: /** @type {import('types').ValidatedConfig} */ (initial)
});
await write_all_types(initial, manifest);
write_all_types(initial, manifest);
}

test('Creates correct $types', async () => {
test('Creates correct $types', () => {
// To save us from creating a real SvelteKit project for each of the tests,
// we first run the type generation directly for each test case, and then
// call `tsc` to check that the generated types are valid.
await run_test('actions');
await run_test('simple-page-shared-only');
await run_test('simple-page-server-only');
await run_test('simple-page-server-and-shared');
await run_test('layout');
await run_test('layout-advanced');
await run_test('slugs');
await run_test('slugs-layout-not-all-pages-have-load');
await run_test('param-type-inference');
run_test('actions');
run_test('simple-page-shared-only');
run_test('simple-page-server-only');
run_test('simple-page-server-and-shared');
run_test('layout');
run_test('layout-advanced');
run_test('slugs');
run_test('slugs-layout-not-all-pages-have-load');
run_test('param-type-inference');
try {
execSync('pnpm testtypes', { cwd });
} catch (e) {
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/src/exports/hooks/sequence.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ test('uses first defined preload option', async () => {
const event = /** @type {import('@sveltejs/kit').RequestEvent} */ ({});
const response = await handler({
event,
resolve: async (_event, opts = {}) => {
resolve: (_event, opts = {}) => {
let html = '';

const { preload = () => false } = opts;
Expand Down Expand Up @@ -153,7 +153,7 @@ test('uses first defined filterSerializedResponseHeaders option', async () => {
const event = /** @type {import('@sveltejs/kit').RequestEvent} */ ({});
const response = await handler({
event,
resolve: async (_event, opts = {}) => {
resolve: (_event, opts = {}) => {
let html = '';

const { filterSerializedResponseHeaders = () => false } = opts;
Expand Down
2 changes: 2 additions & 0 deletions packages/kit/src/exports/node/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ function get_raw_body(req, body_size_limit) {
* }} options
* @returns {Promise<Request>}
*/
// eslint-disable-next-line @typescript-eslint/require-await
export async function getRequest({ request, base, bodySizeLimit }) {
return new Request(base + request.url, {
// @ts-expect-error
Expand All @@ -121,6 +122,7 @@ export async function getRequest({ request, base, bodySizeLimit }) {
* @param {Response} response
* @returns {Promise<void>}
*/
// eslint-disable-next-line @typescript-eslint/require-await
export async function setResponse(res, response) {
for (const [key, value] of response.headers) {
try {
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/src/exports/vite/build/build_service_worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ export async function build_service_worker(
*/
const sw_virtual_modules = {
name: 'service-worker-build-virtual-modules',
async resolveId(id) {
resolveId(id) {
if (id.startsWith('$env/') || id.startsWith('$app/') || id === '$service-worker') {
return `\0virtual:${id}`;
}
},

async load(id) {
load(id) {
if (!id.startsWith('\0virtual:')) return;

if (id === service_worker) {
Expand Down
Loading