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.

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

chore: fix cookie migration hints, and also disable them #11262

Merged
merged 7 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { assert, test } from 'vitest';
import { read_samples } from '../utils.js';
import { migrate_page } from './index.js';
import { read_samples } from '../../../utils.js';

for (const sample of read_samples(import.meta.url)) {
for (const sample of read_samples(new URL('./samples.md', import.meta.url))) {
test(sample.description, () => {
const actual = migrate_page(sample.before, sample.filename ?? '+page.js');
assert.equal(actual, sample.after);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { assert, test } from 'vitest';
import { read_samples } from '../utils.js';
import { migrate_page_server } from './index.js';
import { read_samples } from '../../../utils.js';

for (const sample of read_samples(import.meta.url)) {
for (const sample of read_samples(new URL('./samples.md', import.meta.url))) {
test(sample.description, () => {
const actual = migrate_page_server(sample.before, sample.filename ?? '+page.server.js');
assert.equal(actual, sample.after);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { assert, test } from 'vitest';
import { read_samples } from '../utils.js';
import { migrate_scripts } from './index.js';
import { read_samples } from '../../../utils.js';

for (const sample of read_samples(import.meta.url)) {
for (const sample of read_samples(new URL('./samples.md', import.meta.url))) {
test(sample.description, () => {
const actual = migrate_scripts(
sample.before,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { assert, test } from 'vitest';
import { read_samples } from '../utils.js';
import { migrate_server } from './index.js';
import { read_samples } from '../../../utils.js';

for (const sample of read_samples(import.meta.url)) {
for (const sample of read_samples(new URL('./samples.md', import.meta.url))) {
test(sample.description, () => {
const actual = migrate_server(sample.before);
assert.equal(actual, sample.after);
Expand Down
30 changes: 0 additions & 30 deletions packages/migrate/migrations/routes/utils.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import fs from 'node:fs';
import ts from 'typescript';
import MagicString from 'magic-string';
import { comment, indent_at_line } from '../../utils.js';
Expand Down Expand Up @@ -308,35 +307,6 @@ export function parse(content) {
}
}

/** @param {string} test_file */
export function read_samples(test_file) {
const markdown = fs.readFileSync(new URL('./samples.md', test_file), 'utf8');
const samples = markdown
.split(/^##/gm)
.slice(1)
.map((block) => {
const description = block.split('\n')[0];
const before = /```(js|ts|svelte) before\n([^]*?)\n```/.exec(block);
const after = /```(js|ts|svelte) after\n([^]*?)\n```/.exec(block);

const match = /> file: (.+)/.exec(block);

return {
description,
before: before ? before[2] : '',
after: after ? after[2] : '',
filename: match?.[1],
solo: block.includes('> solo')
};
});

if (samples.some((sample) => sample.solo)) {
return samples.filter((sample) => sample.solo);
}

return samples;
}

/**
* @param {ts.Node} node
* @param {MagicString} code
Expand Down
47 changes: 32 additions & 15 deletions packages/migrate/migrations/sveltekit-2/migrate.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ function add_cookie_note(file_path, source) {
'Remember to add the `path` option to `cookies.set/delete/serialize` calls: https://kit.svelte.dev/docs/v2-migration-guide#path-is-now-a-required-option-for-cookies'
);

const calls = [];

for (const call of source.getDescendantsOfKind(SyntaxKind.CallExpression)) {
const expression = call.getExpression();
if (!Node.isPropertyAccessExpression(expression)) {
Expand All @@ -162,40 +164,55 @@ function add_cookie_note(file_path, source) {
continue;
}

if (call.getText().includes('path:')) {
if (call.getText().includes('path')) {
continue;
}

const expression_text = expression.getExpression().getText();
if (expression_text !== 'cookies') {
const options_arg = call.getArguments()[name === 'delete' ? 1 : 2];
if (options_arg && !Node.isObjectLiteralExpression(options_arg)) {
continue;
}

const some_function = call.getFirstAncestor(
/** @returns {ancestor is import('ts-morph').FunctionDeclaration | import('ts-morph').VariableDeclaration} */
const parent_function = call.getFirstAncestor(
/** @returns {ancestor is import('ts-morph').FunctionDeclaration | import('ts-morph').FunctionExpression | import('ts-morph').ArrowFunction} */
(ancestor) => {
// Check if this is inside a function
const fn = ancestor.asKind(SyntaxKind.FunctionDeclaration);
const arrow_fn = ancestor.asKind(SyntaxKind.VariableDeclaration);
return (
!!fn || (!!arrow_fn && arrow_fn.getInitializer()?.getKind() === SyntaxKind.ArrowFunction)
);
const fn_declaration = ancestor.asKind(SyntaxKind.FunctionDeclaration);
const fn_expression = ancestor.asKind(SyntaxKind.FunctionExpression);
const arrow_fn_expression = ancestor.asKind(SyntaxKind.ArrowFunction);
return !!fn_declaration || !!fn_expression || !!arrow_fn_expression;
}
);
if (!some_function) {
if (!parent_function) {
continue;
}

const expression_text = expression.getExpression().getText();
if (
expression_text !== 'cookies' &&
(!expression_text.includes('.') ||
!parent_function.getParameter(expression_text.split('.')[0]))
) {
continue;
}

const parentStatement = call.getParentIfKind(SyntaxKind.ExpressionStatement);
if (!parentStatement) {
const parent = call.getFirstAncestorByKind(SyntaxKind.Block);
if (!parent) {
continue;
}

parentStatement.replaceWithText(
'/* @migration task: add path argument */' + parentStatement.getText()
calls.push(() =>
call.replaceWithText((writer) => {
writer.setIndentationLevel(0); // prevent ts-morph from being unhelpful and adding its own indentation
writer.write('/* @migration task: add path argument */ ' + call.getText());
})
);
}

for (const call of calls) {
call();
}

logger();
}

Expand Down
180 changes: 26 additions & 154 deletions packages/migrate/migrations/sveltekit-2/migrate.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,157 +4,29 @@ import {
update_svelte_config_content,
update_tsconfig_content
} from './migrate.js';

test('Removes throws#1', () => {
const result = transform_code(
`import { redirect, error } from '@sveltejs/kit';

throw redirect();
redirect();
throw error();
error();
function x() {
let redirect = true;
throw redirect();
}`,
false,
''
);
assert.equal(
result,

`import { redirect, error } from '@sveltejs/kit';

redirect();
redirect();
error();
error();
function x() {
let redirect = true;
throw redirect();
}`
);
});

test('Removes throws#2', () => {
const result = transform_code(
`import { redirect, error } from 'somewhere-else';

throw redirect();
redirect();
throw error();
error();`,
false,
''
);
assert.equal(
result,

`import { redirect, error } from 'somewhere-else';

throw redirect();
redirect();
throw error();
error();`
);
});

test('Notes cookies#1', () => {
const result = transform_code(
`
export function load({ cookies }) {
cookies.set('foo', 'bar');
}`,
false,
'+page.js'
);
assert.equal(
result,

`
export function load({ cookies }) {
/* @migration task: add path argument */cookies.set('foo', 'bar');
}`
);
});

test('Notes cookies#2', () => {
const result = transform_code(
`
export function load({ cookies }) {
cookies.set('foo', 'bar', { path: '/' });
}`,
false,
'+page.js'
);
assert.equal(
result,

`
export function load({ cookies }) {
cookies.set('foo', 'bar', { path: '/' });
}`
);
});

test('Removes old tsconfig options#1', () => {
const result = update_tsconfig_content(
`{
"extends": "./.svelte-kit/tsconfig.json",
"compilerOptions": {
"importsNotUsedAsValues": "error",
"preserveValueImports": true
}
}`
);
assert.equal(
result,

`{
"extends": "./.svelte-kit/tsconfig.json",
"compilerOptions": {
}
}`
);
});

test('Removes old tsconfig options#2', () => {
const result = update_tsconfig_content(
`{
"compilerOptions": {
"importsNotUsedAsValues": "error",
"preserveValueImports": true
}
}`
);
assert.equal(
result,
`{
"compilerOptions": {
"importsNotUsedAsValues": "error",
"preserveValueImports": true
}
}`
);
});

test('Updates svelte.config.js', () => {
const result = update_svelte_config_content(
`export default {
kit: {
foo: bar,
dangerZone: {
trackServerFetches: true
},
baz: qux
}`
);
assert.equal(
result,
`export default {
kit: {
foo: bar,
baz: qux
}`
);
});
import { read_samples } from '../../utils.js';

for (const sample of read_samples(new URL('./svelte-config-samples.md', import.meta.url))) {
test('svelte.config.js: ' + sample.description, () => {
const actual = update_svelte_config_content(sample.before);
assert.equal(actual, sample.after);
});
}

for (const sample of read_samples(new URL('./tsconfig-samples.md', import.meta.url))) {
test('tsconfig.json: ' + sample.description, () => {
const actual = update_tsconfig_content(sample.before);
assert.equal(actual, sample.after);
});
}

for (const sample of read_samples(new URL('./tsjs-samples.md', import.meta.url))) {
test('JS/TS file: ' + sample.description, () => {
const actual = transform_code(
sample.before,
sample.filename?.endsWith('.ts') ?? false,
sample.filename ?? '+page.js'
);
assert.equal(actual, sample.after);
});
}
44 changes: 44 additions & 0 deletions packages/migrate/migrations/sveltekit-2/svelte-config-samples.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
## Removes dangerZone (1)

```js before
export default {
kit: {
foo: bar,
dangerZone: {
trackServerFetches: true
},
baz: qux
}
};
```

```js after
export default {
kit: {
foo: bar,
baz: qux
}
};
```

## Removes dangerZone (2)

```js before
export default {
kit: {
foo: bar,
dangerZone: {
trackServerFetches: true
}
}
};
```

<!-- prettier-ignore -->
```js after
export default {
kit: {
foo: bar,
}
};
```
Loading
Loading