From 18c04463d5ed1977c45a9034daaa5ab268f564b2 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Fri, 5 Aug 2022 13:11:56 +0200 Subject: [PATCH 1/4] [fix handle lone returns, turn on strict mode --- .../routes/migrate_page_js/index.js | 12 +- .../routes/migrate_page_js/samples.md | 17 +- .../routes/migrate_page_server/index.js | 13 +- .../routes/migrate_page_server/samples.md | 17 +- .../routes/migrate_scripts/index.js | 12 +- .../migrations/routes/migrate_server/index.js | 178 +++++++++--------- .../routes/migrate_server/samples.md | 17 +- packages/migrate/migrations/routes/utils.js | 28 ++- packages/migrate/package.json | 3 +- packages/migrate/tsconfig.json | 2 +- 10 files changed, 181 insertions(+), 118 deletions(-) diff --git a/packages/migrate/migrations/routes/migrate_page_js/index.js b/packages/migrate/migrations/routes/migrate_page_js/index.js index aa791328872a..4953d3af13b7 100644 --- a/packages/migrate/migrations/routes/migrate_page_js/index.js +++ b/packages/migrate/migrations/routes/migrate_page_js/index.js @@ -24,21 +24,21 @@ export function migrate_page(content) { const file = parse(content); if (!file) return give_up + content; - if (!file.exports.map.has('load') && !file.exports.complex) { + const name = file.exports.map.get('load'); + + if (!name && !file.exports.complex) { // there's no load function here, so there's nothing to do return content; } - const name = file.exports.map.get('load'); - for (const statement of file.ast.statements) { - const fn = get_function_node(statement, name); - if (fn) { + const fn = name ? get_function_node(statement, name) : undefined; + if (fn?.body) { /** @type {Set} */ const imports = new Set(); rewrite_returns(fn.body, (expr, node) => { - const nodes = ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); + const nodes = expr && ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); if (nodes) { const keys = Object.keys(nodes).sort().join(' '); diff --git a/packages/migrate/migrations/routes/migrate_page_js/samples.md b/packages/migrate/migrations/routes/migrate_page_js/samples.md index 9dcb5194b2a5..089b5f6106d1 100644 --- a/packages/migrate/migrations/routes/migrate_page_js/samples.md +++ b/packages/migrate/migrations/routes/migrate_page_js/samples.md @@ -235,4 +235,19 @@ export async function load({ fetch }) { throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292693)"); return await res.json(); } -``` \ No newline at end of file +``` + +## A load function that returns nothing + +```js before +export function load() { + return; +} +``` + +```js after +export function load() { + throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292693)"); + return; +} +``` diff --git a/packages/migrate/migrations/routes/migrate_page_server/index.js b/packages/migrate/migrations/routes/migrate_page_server/index.js index 30f5c362cd73..04421913b0c8 100644 --- a/packages/migrate/migrations/routes/migrate_page_server/index.js +++ b/packages/migrate/migrations/routes/migrate_page_server/index.js @@ -27,12 +27,13 @@ export function migrate_page_server(content) { const unmigrated = new Set(methods); for (const statement of file.ast.statements) { - if (file.exports.map.has('GET')) { - const GET = get_function_node(statement, file.exports.map.get('GET')); - if (GET) { + const GET_id = file.exports.map.get('GET'); + if (GET_id) { + const GET = get_function_node(statement, GET_id); + if (GET?.body) { // possible TODOs — handle errors and redirects rewrite_returns(GET.body, (expr, node) => { - const nodes = ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); + const nodes = expr && ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); if (!nodes || nodes.headers || (nodes.status && nodes.status.getText() !== '200')) { manual_return_migration(node || GET, file.code, TASKS.PAGE_ENDPOINT); @@ -47,8 +48,8 @@ export function migrate_page_server(content) { } for (const method of non_get_methods) { - const fn = get_function_node(statement, file.exports.map.get(method)); - if (fn) { + const fn = get_function_node(statement, /** @type{string} */ (file.exports.map.get(method))); + if (fn?.body) { rewrite_returns(fn.body, (expr, node) => { manual_return_migration(node || fn, file.code, TASKS.PAGE_ENDPOINT); }); diff --git a/packages/migrate/migrations/routes/migrate_page_server/samples.md b/packages/migrate/migrations/routes/migrate_page_server/samples.md index bd8b46964171..fe4d64e5686d 100644 --- a/packages/migrate/migrations/routes/migrate_page_server/samples.md +++ b/packages/migrate/migrations/routes/migrate_page_server/samples.md @@ -68,4 +68,19 @@ export function POST() { throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292699)"); return {}; } -``` \ No newline at end of file +``` + +## A function that returns nothing + +```js before +export function GET() { + return; +} +``` + +```js after +export function GET() { + throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292699)"); + return; +} +``` diff --git a/packages/migrate/migrations/routes/migrate_scripts/index.js b/packages/migrate/migrations/routes/migrate_scripts/index.js index 46b8e5432b4e..3a9a40f69e95 100644 --- a/packages/migrate/migrations/routes/migrate_scripts/index.js +++ b/packages/migrate/migrations/routes/migrate_scripts/index.js @@ -46,7 +46,7 @@ export function migrate_scripts(content, is_error, moved) { const delete_var = (/** @type {string } */ key) => { const declaration = declared?.get(key); if (declaration && !declaration.import) { - declared.delete(key); + declared?.delete(key); } }; delete_var('load'); @@ -61,7 +61,7 @@ export function migrate_scripts(content, is_error, moved) { declaration.import.from === '@sveltejs/kit' && !new RegExp(`\\W${key}\\W`).test(except_str(content, match)) ) { - declared.delete(key); + declared?.delete(key); } }; delete_kit_type('Load'); @@ -98,7 +98,7 @@ function find_declarations(content) { const file = parse(content); if (!file) return; - /** @type {Map} */ + /** @type {Map} */ const declared = new Map(); /** * @param {string} name @@ -111,7 +111,9 @@ function find_declarations(content) { for (const statement of file.ast.statements) { if (ts.isImportDeclaration(statement) && statement.importClause) { let type_only = statement.importClause.isTypeOnly; - const from = ts.isStringLiteral(statement.moduleSpecifier) && statement.moduleSpecifier.text; + const from = ts.isStringLiteral(statement.moduleSpecifier) + ? statement.moduleSpecifier.text + : ''; if (statement.importClause.name) { add(statement.importClause.name.text, { from, type_only }); @@ -137,7 +139,7 @@ function find_declarations(content) { } } } else if (ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) { - if (ts.isIdentifier(statement.name)) { + if (statement.name && ts.isIdentifier(statement.name)) { add(statement.name.text); } } else if (ts.isExportDeclaration(statement) && !statement.exportClause) { diff --git a/packages/migrate/migrations/routes/migrate_server/index.js b/packages/migrate/migrations/routes/migrate_server/index.js index 298666b45741..57bcb539f819 100644 --- a/packages/migrate/migrations/routes/migrate_server/index.js +++ b/packages/migrate/migrations/routes/migrate_server/index.js @@ -30,102 +30,104 @@ export function migrate_server(content) { for (const statement of file.ast.statements) { for (const method of methods) { - const fn = get_function_node(statement, file.exports.map.get(method)); - if (fn) { + const fn = get_function_node(statement, /** @type{string} */ (file.exports.map.get(method))); + if (fn?.body) { rewrite_returns(fn.body, (expr, node) => { - // leave `() => new Response(...)` alone - if (is_new(expr, 'Response')) return; - - const nodes = ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); - - if (nodes) { - const body_is_object_literal = nodes.body && ts.isObjectLiteralExpression(nodes.body); - - let safe_headers = !nodes.headers; - if (nodes.headers) { - if (ts.isObjectLiteralExpression(nodes.headers)) { - // if `headers` is an object literal, and it either doesn't contain - // `set-cookie` or `set-cookie` is a string, then the headers - // are safe to use in a `Response` - const set_cookie_value = nodes.headers.properties.find((prop) => { - return ( - ts.isPropertyAssignment(prop) && - ts.isStringLiteral(prop.name) && - /set-cookie/i.test(prop.name.text) - ); - }); - - if (!set_cookie_value || is_string_like(set_cookie_value)) { - safe_headers = true; + if (expr) { + // leave `() => new Response(...)` alone + if (is_new(expr, 'Response')) return; + + const nodes = ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); + + if (nodes) { + const body_is_object_literal = nodes.body && ts.isObjectLiteralExpression(nodes.body); + + let safe_headers = !nodes.headers; + if (nodes.headers) { + if (ts.isObjectLiteralExpression(nodes.headers)) { + // if `headers` is an object literal, and it either doesn't contain + // `set-cookie` or `set-cookie` is a string, then the headers + // are safe to use in a `Response` + const set_cookie_value = nodes.headers.properties.find((prop) => { + return ( + ts.isPropertyAssignment(prop) && + ts.isStringLiteral(prop.name) && + /set-cookie/i.test(prop.name.text) + ); + }); + + if (!set_cookie_value || is_string_like(set_cookie_value)) { + safe_headers = true; + } + } else { + // `headers: new Headers(...)` is also safe, as long as we + // don't need to augment it with `content-type` + safe_headers = is_new(nodes.headers, 'Headers') && !body_is_object_literal; } - } else { - // `headers: new Headers(...)` is also safe, as long as we - // don't need to augment it with `content-type` - safe_headers = is_new(nodes.headers, 'Headers') && !body_is_object_literal; } - } - const safe_body = - !nodes.body || - is_string_like(nodes.body) || - body_is_object_literal || - (ts.isCallExpression(nodes.body) && - nodes.body.expression.getText() === 'JSON.stringify'); - - if (safe_headers) { - let status = nodes.status ? nodes.status.getText() : '200'; - let headers = nodes.headers?.getText(); - let body = dedent(nodes.body?.getText() || 'undefined'); - - const multiline = /\n/.test(headers); - - if (body_is_object_literal || (nodes.body && ts.isIdentifier(nodes.body))) { - // `return { body: {...} }` is safe to convert to a JSON response, - // but we probably need to add a `content-type` header - body = `JSON.stringify(${body})`; - const header = `'content-type': 'application/json; charset=utf-8'`; - if ( - nodes.headers && - ts.isObjectLiteralExpression(nodes.headers) && - nodes.headers.properties.length > 0 - ) { - const join = multiline - ? `,\n${indent_at_line(content, nodes.headers.properties[0].getStart())}` - : `, `; - headers = headers.replace(/[{\s]/, header + join); + const safe_body = + !nodes.body || + is_string_like(nodes.body) || + body_is_object_literal || + (ts.isCallExpression(nodes.body) && + nodes.body.expression.getText() === 'JSON.stringify'); + + if (safe_headers) { + let status = nodes.status ? nodes.status.getText() : '200'; + let headers = nodes.headers?.getText(); + let body = dedent(nodes.body?.getText() || 'undefined'); + + const multiline = /\n/.test(headers); + + if (body_is_object_literal || (nodes.body && ts.isIdentifier(nodes.body))) { + // `return { body: {...} }` is safe to convert to a JSON response, + // but we probably need to add a `content-type` header + body = `JSON.stringify(${body})`; + const header = `'content-type': 'application/json; charset=utf-8'`; + if ( + nodes.headers && + ts.isObjectLiteralExpression(nodes.headers) && + nodes.headers.properties.length > 0 + ) { + const join = multiline + ? `,\n${indent_at_line(content, nodes.headers.properties[0].getStart())}` + : `, `; + headers = headers.replace(/[{\s]/, header + join); + } else { + headers = `{ ${header} }`; + } + } + + const init = [ + status !== '200' && `status: ${status}`, + headers && `headers: ${headers}` + ].filter(Boolean); + + const indent = indent_at_line(content, expr.getStart()); + const end_whitespace = multiline ? `\n${indent}` : ' '; + const join_whitespace = multiline ? end_whitespace + guess_indent(content) : ' '; + + const response = + init.length > 0 + ? `new Response(${body}, {${join_whitespace}${init.join( + `,${join_whitespace}` + )}${end_whitespace}})` + : `new Response(${body})`; + + if (safe_body) { + automigration(expr, file.code, response); } else { - headers = `{ ${header} }`; + manual_return_migration( + node || fn, + file.code, + TASKS.STANDALONE_ENDPOINT, + `return ${response};` + ); } - } - const init = [ - status !== '200' && `status: ${status}`, - headers && `headers: ${headers}` - ].filter(Boolean); - - const indent = indent_at_line(content, expr.getStart()); - const end_whitespace = multiline ? `\n${indent}` : ' '; - const join_whitespace = multiline ? end_whitespace + guess_indent(content) : ' '; - - const response = - init.length > 0 - ? `new Response(${body}, {${join_whitespace}${init.join( - `,${join_whitespace}` - )}${end_whitespace}})` - : `new Response(${body})`; - - if (safe_body) { - automigration(expr, file.code, response); - } else { - manual_return_migration( - node || fn, - file.code, - TASKS.STANDALONE_ENDPOINT, - `return ${response};` - ); + return; } - - return; } } diff --git a/packages/migrate/migrations/routes/migrate_server/samples.md b/packages/migrate/migrations/routes/migrate_server/samples.md index f20fa078a46f..c789714d2c32 100644 --- a/packages/migrate/migrations/routes/migrate_server/samples.md +++ b/packages/migrate/migrations/routes/migrate_server/samples.md @@ -112,4 +112,19 @@ export const GET = () => createResponse('text'); ```js after throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292701)"); export const GET = () => createResponse('text'); -``` \ No newline at end of file +``` + +## A function that returns nothing + +```js before +export function GET() { + return; +} +``` + +```js after +export function GET() { + throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292701)"); + return; +} +``` diff --git a/packages/migrate/migrations/routes/utils.js b/packages/migrate/migrations/routes/utils.js index bc6a27563e25..c8124c116d3c 100644 --- a/packages/migrate/migrations/routes/utils.js +++ b/packages/migrate/migrations/routes/utils.js @@ -179,7 +179,7 @@ export function guess_indent(content) { // Otherwise, we need to guess the multiple const min = spaced.reduce((previous, current) => { - const count = /^ +/.exec(current)[0].length; + const count = /^ +/.exec(current)?.[0].length ?? 0; return Math.min(count, previous); }, Infinity); @@ -192,7 +192,7 @@ export function guess_indent(content) { */ export function indent_at_line(content, offset) { const substr = content.substring(content.lastIndexOf('\n', offset) + 1, offset); - return /\s*/.exec(substr)[0]; + return /\s*/.exec(substr)?.[0] ?? ''; } /** @@ -274,13 +274,18 @@ export function get_exports(node) { let complex = false; for (const statement of node.statements) { - if (ts.isExportDeclaration(statement) && ts.isNamedExports(statement.exportClause)) { + if ( + ts.isExportDeclaration(statement) && + statement.exportClause && + ts.isNamedExports(statement.exportClause) + ) { // export { x }, export { x as y } for (const specifier of statement.exportClause.elements) { map.set(specifier.name.text, specifier.propertyName?.text || specifier.name.text); } } else if ( ts.isFunctionDeclaration(statement) && + statement.name && statement.modifiers?.[0]?.kind === ts.SyntaxKind.ExportKeyword ) { // export function x ... @@ -311,7 +316,11 @@ export function get_exports(node) { * @returns {ts.FunctionDeclaration | ts.FunctionExpression | ts.ArrowFunction | void} */ export function get_function_node(statement, ...names) { - if (ts.isFunctionDeclaration(statement) && names.includes(statement.name.text)) { + if ( + ts.isFunctionDeclaration(statement) && + statement.name && + names.includes(statement.name.text) + ) { // export function x ... return statement; } @@ -321,6 +330,7 @@ export function get_function_node(statement, ...names) { if ( ts.isIdentifier(declaration.name) && names.includes(declaration.name.text) && + declaration.initializer && (ts.isArrowFunction(declaration.initializer) || ts.isFunctionExpression(declaration.initializer)) ) { @@ -332,10 +342,12 @@ export function get_function_node(statement, ...names) { } /** - * Utility for rewriting return statements. If `node` is `undefined`, - * it means it's a concise arrow function body (`() => ({}))` + * Utility for rewriting return statements. + * If `node` is `undefined`, it means it's a concise arrow function body (`() => ({}))`. + * If `expression` is `undefined`, it means it's a lone return statement `return;`. + * Both can't be `undefined` at the same time. * @param {ts.Block | ts.ConciseBody} block - * @param {(expression: ts.Expression, node: ts.ReturnStatement | void) => void} callback + * @param {(expression: ts.Expression | undefined, node: ts.ReturnStatement | undefined) => void} callback */ export function rewrite_returns(block, callback) { if (ts.isBlock(block)) { @@ -364,7 +376,7 @@ export function rewrite_returns(block, callback) { block = block.expression; } - callback(block, null); + callback(block, undefined); } } diff --git a/packages/migrate/package.json b/packages/migrate/package.json index 408bb2e585dc..b8407a94a2cc 100644 --- a/packages/migrate/package.json +++ b/packages/migrate/package.json @@ -32,6 +32,7 @@ "uvu": "^0.5.6" }, "scripts": { - "test": "uvu" + "test": "uvu", + "check": "tsc" } } diff --git a/packages/migrate/tsconfig.json b/packages/migrate/tsconfig.json index 935dd8553604..07a6cc58c168 100644 --- a/packages/migrate/tsconfig.json +++ b/packages/migrate/tsconfig.json @@ -3,7 +3,7 @@ "allowJs": true, "checkJs": true, "noEmit": true, - "noImplicitAny": true, + "strict": true, "target": "esnext", "module": "esnext", "moduleResolution": "node", From 018df4c40245259e5145bdee496b6c2f2712c880 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Fri, 5 Aug 2022 13:18:02 +0200 Subject: [PATCH 2/4] this changeset name lol --- .changeset/pretty-teachers-smash.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/pretty-teachers-smash.md diff --git a/.changeset/pretty-teachers-smash.md b/.changeset/pretty-teachers-smash.md new file mode 100644 index 000000000000..2a8e5d7529ea --- /dev/null +++ b/.changeset/pretty-teachers-smash.md @@ -0,0 +1,5 @@ +--- +'svelte-migrate': patch +--- + +handle lone return statements From 960fe177c2e8dc626ee7453708638ad8ca89f208 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Sat, 6 Aug 2022 20:14:24 +0200 Subject: [PATCH 3/4] skip lone returns --- .../migrate/migrations/routes/migrate_page_js/samples.md | 1 - .../migrations/routes/migrate_page_server/samples.md | 1 - .../migrate/migrations/routes/migrate_server/samples.md | 1 - packages/migrate/migrations/routes/utils.js | 9 ++++----- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/migrate/migrations/routes/migrate_page_js/samples.md b/packages/migrate/migrations/routes/migrate_page_js/samples.md index f9dde56c1df1..2c52826d8218 100644 --- a/packages/migrate/migrations/routes/migrate_page_js/samples.md +++ b/packages/migrate/migrations/routes/migrate_page_js/samples.md @@ -291,7 +291,6 @@ export function load() { ```js after export function load() { - throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292693)"); return; } ``` diff --git a/packages/migrate/migrations/routes/migrate_page_server/samples.md b/packages/migrate/migrations/routes/migrate_page_server/samples.md index fe4d64e5686d..1a8587b73e94 100644 --- a/packages/migrate/migrations/routes/migrate_page_server/samples.md +++ b/packages/migrate/migrations/routes/migrate_page_server/samples.md @@ -80,7 +80,6 @@ export function GET() { ```js after export function GET() { - throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292699)"); return; } ``` diff --git a/packages/migrate/migrations/routes/migrate_server/samples.md b/packages/migrate/migrations/routes/migrate_server/samples.md index c789714d2c32..e2fe511bba4a 100644 --- a/packages/migrate/migrations/routes/migrate_server/samples.md +++ b/packages/migrate/migrations/routes/migrate_server/samples.md @@ -124,7 +124,6 @@ export function GET() { ```js after export function GET() { - throw new Error("@migration task: Migrate this return statement (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292701)"); return; } ``` diff --git a/packages/migrate/migrations/routes/utils.js b/packages/migrate/migrations/routes/utils.js index 59915fa12ffa..3f6dda59d7f0 100644 --- a/packages/migrate/migrations/routes/utils.js +++ b/packages/migrate/migrations/routes/utils.js @@ -321,7 +321,7 @@ export function get_exports(node) { /** * @param {ts.Node} statement * @param {string[]} names - * @returns {ts.FunctionDeclaration | ts.FunctionExpression | ts.ArrowFunction | void} + * @returns {ts.FunctionDeclaration | ts.FunctionExpression | ts.ArrowFunction | undefined} */ export function get_function_node(statement, ...names) { if ( @@ -352,10 +352,9 @@ export function get_function_node(statement, ...names) { /** * Utility for rewriting return statements. * If `node` is `undefined`, it means it's a concise arrow function body (`() => ({}))`. - * If `expression` is `undefined`, it means it's a lone return statement `return;`. - * Both can't be `undefined` at the same time. + * Lone `return;` statements are left untouched. * @param {ts.Block | ts.ConciseBody} block - * @param {(expression: ts.Expression | undefined, node: ts.ReturnStatement | undefined) => void} callback + * @param {(expression: ts.Expression, node: ts.ReturnStatement | undefined) => void} callback */ export function rewrite_returns(block, callback) { if (ts.isBlock(block)) { @@ -370,7 +369,7 @@ export function rewrite_returns(block, callback) { return; } - if (ts.isReturnStatement(node)) { + if (ts.isReturnStatement(node) && node.expression) { callback(node.expression, node); return; } From 4bdfeda46b44f246ecdc7700257378462f716d32 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Sat, 6 Aug 2022 20:23:15 +0200 Subject: [PATCH 4/4] simplify, now that expr is always defined --- .../routes/migrate_page_js/index.js | 2 +- .../routes/migrate_page_server/index.js | 2 +- .../migrations/routes/migrate_server/index.js | 174 +++++++++--------- 3 files changed, 88 insertions(+), 90 deletions(-) diff --git a/packages/migrate/migrations/routes/migrate_page_js/index.js b/packages/migrate/migrations/routes/migrate_page_js/index.js index 074db0ad002b..7c348774b588 100644 --- a/packages/migrate/migrations/routes/migrate_page_js/index.js +++ b/packages/migrate/migrations/routes/migrate_page_js/index.js @@ -41,7 +41,7 @@ export function migrate_page(content) { const imports = new Set(); rewrite_returns(fn.body, (expr, node) => { - const nodes = expr && ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); + const nodes = ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); if (nodes) { const keys = Object.keys(nodes).sort().join(' '); diff --git a/packages/migrate/migrations/routes/migrate_page_server/index.js b/packages/migrate/migrations/routes/migrate_page_server/index.js index 04421913b0c8..bb616991f8ca 100644 --- a/packages/migrate/migrations/routes/migrate_page_server/index.js +++ b/packages/migrate/migrations/routes/migrate_page_server/index.js @@ -33,7 +33,7 @@ export function migrate_page_server(content) { if (GET?.body) { // possible TODOs — handle errors and redirects rewrite_returns(GET.body, (expr, node) => { - const nodes = expr && ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); + const nodes = ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); if (!nodes || nodes.headers || (nodes.status && nodes.status.getText() !== '200')) { manual_return_migration(node || GET, file.code, TASKS.PAGE_ENDPOINT); diff --git a/packages/migrate/migrations/routes/migrate_server/index.js b/packages/migrate/migrations/routes/migrate_server/index.js index 57bcb539f819..4668eaf1bbc4 100644 --- a/packages/migrate/migrations/routes/migrate_server/index.js +++ b/packages/migrate/migrations/routes/migrate_server/index.js @@ -33,101 +33,99 @@ export function migrate_server(content) { const fn = get_function_node(statement, /** @type{string} */ (file.exports.map.get(method))); if (fn?.body) { rewrite_returns(fn.body, (expr, node) => { - if (expr) { - // leave `() => new Response(...)` alone - if (is_new(expr, 'Response')) return; - - const nodes = ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); - - if (nodes) { - const body_is_object_literal = nodes.body && ts.isObjectLiteralExpression(nodes.body); - - let safe_headers = !nodes.headers; - if (nodes.headers) { - if (ts.isObjectLiteralExpression(nodes.headers)) { - // if `headers` is an object literal, and it either doesn't contain - // `set-cookie` or `set-cookie` is a string, then the headers - // are safe to use in a `Response` - const set_cookie_value = nodes.headers.properties.find((prop) => { - return ( - ts.isPropertyAssignment(prop) && - ts.isStringLiteral(prop.name) && - /set-cookie/i.test(prop.name.text) - ); - }); - - if (!set_cookie_value || is_string_like(set_cookie_value)) { - safe_headers = true; - } - } else { - // `headers: new Headers(...)` is also safe, as long as we - // don't need to augment it with `content-type` - safe_headers = is_new(nodes.headers, 'Headers') && !body_is_object_literal; - } - } + // leave `() => new Response(...)` alone + if (is_new(expr, 'Response')) return; + + const nodes = ts.isObjectLiteralExpression(expr) && get_object_nodes(expr); + + if (nodes) { + const body_is_object_literal = nodes.body && ts.isObjectLiteralExpression(nodes.body); + + let safe_headers = !nodes.headers; + if (nodes.headers) { + if (ts.isObjectLiteralExpression(nodes.headers)) { + // if `headers` is an object literal, and it either doesn't contain + // `set-cookie` or `set-cookie` is a string, then the headers + // are safe to use in a `Response` + const set_cookie_value = nodes.headers.properties.find((prop) => { + return ( + ts.isPropertyAssignment(prop) && + ts.isStringLiteral(prop.name) && + /set-cookie/i.test(prop.name.text) + ); + }); - const safe_body = - !nodes.body || - is_string_like(nodes.body) || - body_is_object_literal || - (ts.isCallExpression(nodes.body) && - nodes.body.expression.getText() === 'JSON.stringify'); - - if (safe_headers) { - let status = nodes.status ? nodes.status.getText() : '200'; - let headers = nodes.headers?.getText(); - let body = dedent(nodes.body?.getText() || 'undefined'); - - const multiline = /\n/.test(headers); - - if (body_is_object_literal || (nodes.body && ts.isIdentifier(nodes.body))) { - // `return { body: {...} }` is safe to convert to a JSON response, - // but we probably need to add a `content-type` header - body = `JSON.stringify(${body})`; - const header = `'content-type': 'application/json; charset=utf-8'`; - if ( - nodes.headers && - ts.isObjectLiteralExpression(nodes.headers) && - nodes.headers.properties.length > 0 - ) { - const join = multiline - ? `,\n${indent_at_line(content, nodes.headers.properties[0].getStart())}` - : `, `; - headers = headers.replace(/[{\s]/, header + join); - } else { - headers = `{ ${header} }`; - } + if (!set_cookie_value || is_string_like(set_cookie_value)) { + safe_headers = true; } + } else { + // `headers: new Headers(...)` is also safe, as long as we + // don't need to augment it with `content-type` + safe_headers = is_new(nodes.headers, 'Headers') && !body_is_object_literal; + } + } - const init = [ - status !== '200' && `status: ${status}`, - headers && `headers: ${headers}` - ].filter(Boolean); - - const indent = indent_at_line(content, expr.getStart()); - const end_whitespace = multiline ? `\n${indent}` : ' '; - const join_whitespace = multiline ? end_whitespace + guess_indent(content) : ' '; - - const response = - init.length > 0 - ? `new Response(${body}, {${join_whitespace}${init.join( - `,${join_whitespace}` - )}${end_whitespace}})` - : `new Response(${body})`; - - if (safe_body) { - automigration(expr, file.code, response); + const safe_body = + !nodes.body || + is_string_like(nodes.body) || + body_is_object_literal || + (ts.isCallExpression(nodes.body) && + nodes.body.expression.getText() === 'JSON.stringify'); + + if (safe_headers) { + let status = nodes.status ? nodes.status.getText() : '200'; + let headers = nodes.headers?.getText(); + let body = dedent(nodes.body?.getText() || 'undefined'); + + const multiline = /\n/.test(headers); + + if (body_is_object_literal || (nodes.body && ts.isIdentifier(nodes.body))) { + // `return { body: {...} }` is safe to convert to a JSON response, + // but we probably need to add a `content-type` header + body = `JSON.stringify(${body})`; + const header = `'content-type': 'application/json; charset=utf-8'`; + if ( + nodes.headers && + ts.isObjectLiteralExpression(nodes.headers) && + nodes.headers.properties.length > 0 + ) { + const join = multiline + ? `,\n${indent_at_line(content, nodes.headers.properties[0].getStart())}` + : `, `; + headers = headers.replace(/[{\s]/, header + join); } else { - manual_return_migration( - node || fn, - file.code, - TASKS.STANDALONE_ENDPOINT, - `return ${response};` - ); + headers = `{ ${header} }`; } + } - return; + const init = [ + status !== '200' && `status: ${status}`, + headers && `headers: ${headers}` + ].filter(Boolean); + + const indent = indent_at_line(content, expr.getStart()); + const end_whitespace = multiline ? `\n${indent}` : ' '; + const join_whitespace = multiline ? end_whitespace + guess_indent(content) : ' '; + + const response = + init.length > 0 + ? `new Response(${body}, {${join_whitespace}${init.join( + `,${join_whitespace}` + )}${end_whitespace}})` + : `new Response(${body})`; + + if (safe_body) { + automigration(expr, file.code, response); + } else { + manual_return_migration( + node || fn, + file.code, + TASKS.STANDALONE_ENDPOINT, + `return ${response};` + ); } + + return; } }