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

Better error logging #1062

Merged
merged 8 commits into from
Apr 17, 2021
Merged

Better error logging #1062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
42271f2
tidy up render options, show parent of erroring page in prerender
Rich-Harris Apr 17, 2021
758f2f8
separate out render options from render state
Rich-Harris Apr 17, 2021
858095b
small tidy up
Rich-Harris Apr 17, 2021
0732a02
fix
Rich-Harris Apr 17, 2021
4627881
always log caught errors when rendering
Rich-Harris Apr 17, 2021
100d2dc
changeset
Rich-Harris Apr 17, 2021
af468b9
fix test, apply stack fix to non-thrown errors returned from load
Rich-Harris Apr 17, 2021
5179338
squelch console errors, now that we generate a bunch more of them
Rich-Harris Apr 17, 2021
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
5 changes: 5 additions & 0 deletions .changeset/lazy-actors-add.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

Better error logging
45 changes: 25 additions & 20 deletions packages/kit/src/core/adapt/prerender.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,17 @@ export async function prerender({ cwd, out, log, config, build_data, force }) {

app.init({
paths: config.kit.paths,
prerendering: true
prerendering: true,
read: (file) => readFileSync(join(config.kit.files.assets, file))
});

/** @type {(status: number, path: string) => void} */
/** @type {(status: number, path: string, parent: string, verb: string) => void} */
const error = config.kit.prerender.force
? (status, path) => {
log.error(`${status} ${path}`);
? (status, path, parent, verb) => {
log.error(`${status} ${path} (${verb} from ${parent})`);
}
: (status, path) => {
throw new Error(`${status} ${path}`);
: (status, path, parent, verb) => {
throw new Error(`${status} ${path} (${verb} from ${parent})`);
};

const files = new Set([...build_data.static, ...build_data.client]);
Expand All @@ -86,8 +87,11 @@ export async function prerender({ cwd, out, log, config, build_data, force }) {
}
});

/** @param {string} path */
async function visit(path) {
/**
* @param {string} path
* @param {string} parent
*/
async function visit(path, parent) {
Copy link
Member

Choose a reason for hiding this comment

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

parent_path might be a clearer name? Or could update the JSDoc to clarify that it's a path

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 feel like it's obvious from usage. If you go by the name alone then you might think that the parent_path of /foo/bar was /foo, whereas if you scan down to where it's used, it becomes clear what it does

if (seen.has(path)) return;
seen.add(path);

Expand All @@ -104,10 +108,11 @@ export async function prerender({ cwd, out, log, config, build_data, force }) {
query: new URLSearchParams()
},
{
local: true,
dependencies,
only_render_prerenderable_pages: !force,
get_static_file: (file) => readFileSync(join(config.kit.files.assets, file))
prerender: {
force,
dependencies,
error: null
}
}
);

Expand Down Expand Up @@ -138,15 +143,15 @@ export async function prerender({ cwd, out, log, config, build_data, force }) {
log.info(`${rendered.status} ${path}`);
writeFileSync(file, rendered.body); // TODO minify where possible?
} else if (response_type !== OK) {
error(rendered.status, path);
error(rendered.status, path, parent, 'linked');
}

dependencies.forEach((result, path) => {
dependencies.forEach((result, dependency_path) => {
const response_type = Math.floor(result.status / 100);

const is_html = result.headers['content-type'] === 'text/html';

const parts = path.split('/');
const parts = dependency_path.split('/');
if (is_html && parts[parts.length - 1] !== 'index.html') {
parts.push('index.html');
}
Expand All @@ -157,9 +162,9 @@ export async function prerender({ cwd, out, log, config, build_data, force }) {
writeFileSync(file, result.body);

if (response_type === OK) {
log.info(`${result.status} ${path}`);
log.info(`${result.status} ${dependency_path}`);
} else {
error(result.status, path);
error(result.status, dependency_path, path, 'fetched');
}
});

Expand Down Expand Up @@ -200,7 +205,7 @@ export async function prerender({ cwd, out, log, config, build_data, force }) {
// TODO warn that query strings have no effect on statically-exported pages
}

await visit(parsed.pathname.replace(config.kit.paths.base, ''));
await visit(parsed.pathname.replace(config.kit.paths.base, ''), path);
}
}
}
Expand All @@ -209,10 +214,10 @@ export async function prerender({ cwd, out, log, config, build_data, force }) {
for (const entry of config.kit.prerender.pages) {
if (entry === '*') {
for (const entry of build_data.entries) {
await visit(entry);
await visit(entry, null);
}
} else {
await visit(entry);
await visit(entry, null);
}
}
}
76 changes: 39 additions & 37 deletions packages/kit/src/core/build/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,12 +279,42 @@ async function build_server(
.replace('%svelte.head%', '" + head + "')
.replace('%svelte.body%', '" + body + "')};
set_paths(${s(config.kit.paths)});
let options = null;
// allow paths to be overridden in svelte-kit start
export function init({ paths, prerendering }) {
set_paths(paths);
set_prerendering(prerendering);
// and in prerendering
export function init(settings) {
set_paths(settings.paths);
set_prerendering(settings.prerendering || false);
options = {
amp: ${config.kit.amp},
dev: false,
entry: {
file: ${s(prefix + client_manifest[client_entry_file].file)},
css: ${s(Array.from(entry_css).map(dep => prefix + dep))},
js: ${s(Array.from(entry_js).map(dep => prefix + dep))}
},
fetched: undefined,
get_component_path: id => ${s(`${config.kit.paths.assets}/${config.kit.appDir}/`)} + entry_lookup[id],
get_stack: error => String(error), // for security
handle_error: error => {
console.error(error.stack);
error.stack = options.get_stack(error);
},
hooks: get_hooks(user_hooks),
hydrate: ${s(config.kit.hydrate)},
initiator: undefined,
load_component,
manifest,
paths: settings.paths,
read: settings.read,
root,
router: ${s(config.kit.router)},
ssr: ${s(config.kit.ssr)},
target: ${s(config.kit.target)},
template
};
}
const d = decodeURIComponent;
Expand Down Expand Up @@ -331,8 +361,6 @@ async function build_server(
handle: hooks.handle || (({ request, render }) => render(request))
});
const hooks = get_hooks(user_hooks);
const module_lookup = {
${manifest.components.map(file => `${s(file)}: () => import(${s(app_relative(file))})`)}
};
Expand All @@ -349,39 +377,13 @@ async function build_server(
};
}
init({ paths: ${s(config.kit.paths)} });
export function render(request, {
paths = ${s(config.kit.paths)},
local = false,
dependencies,
only_render_prerenderable_pages = false,
get_static_file
prerender
} = {}) {
return ssr({
...request,
host: ${config.kit.host ? s(config.kit.host) : `request.headers[${s(config.kit.hostHeader || 'host')}]`}
}, {
paths,
local,
template,
manifest,
load_component,
target: ${s(config.kit.target)},
entry: ${s(prefix + client_manifest[client_entry_file].file)},
css: ${s(Array.from(entry_css).map(dep => prefix + dep))},
js: ${s(Array.from(entry_js).map(dep => prefix + dep))},
root,
hooks,
dev: false,
amp: ${config.kit.amp},
dependencies,
only_render_prerenderable_pages,
get_component_path: id => ${s(`${config.kit.paths.assets}/${config.kit.appDir}/`)} + entry_lookup[id],
get_stack: error => error.stack,
get_static_file,
ssr: ${s(config.kit.ssr)},
router: ${s(config.kit.router)},
hydrate: ${s(config.kit.hydrate)}
});
const host = ${config.kit.host ? s(config.kit.host) : `request.headers[${s(config.kit.hostHeader || 'host')}]`};
return ssr({ ...request, host }, options, { prerender });
}
`
.replace(/^\t{3}/gm, '')
Expand Down
138 changes: 71 additions & 67 deletions packages/kit/src/core/dev/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { EventEmitter } from 'events';
import CheapWatch from 'cheap-watch';
import amp_validator from 'amphtml-validator';
import vite from 'vite';
import colors from 'kleur';
import create_manifest_data from '../../core/create_manifest_data/index.js';
import { create_app } from '../../core/create_app/index.js';
import { rimraf } from '../filesystem/index.js';
Expand Down Expand Up @@ -130,9 +131,6 @@ class Watcher extends EventEmitter {

if (req.url === '/favicon.ico') return;

// handle dynamic requests - i.e. pages and endpoints
const template = fs.readFileSync(this.config.kit.files.template, 'utf-8');

const hooks = /** @type {import('types/internal').Hooks} */ (await this.vite
.ssrLoadModule(`/${this.config.kit.files.hooks}`)
.catch(() => ({})));
Expand Down Expand Up @@ -162,9 +160,77 @@ class Watcher extends EventEmitter {
body
},
{
amp: this.config.kit.amp,
dev: true,
entry: {
file: '/.svelte/dev/runtime/internal/start.js',
css: [],
js: []
},
get_stack: (error) => {
this.vite.ssrFixStacktrace(error);
return error.stack;
},
handle_error: (error) => {
this.vite.ssrFixStacktrace(error);
console.error(colors.bold().red(error.message));
console.error(colors.gray(error.stack));
},
hooks: {
getContext: hooks.getContext || (() => ({})),
getSession: hooks.getSession || (() => ({})),
handle: hooks.handle || (({ request, render }) => render(request))
},
hydrate: this.config.kit.hydrate,
paths: this.config.kit.paths,
load_component: async (id) => {
const url = path.resolve(this.cwd, id);

const module = /** @type {SSRComponent} */ (await this.vite.ssrLoadModule(url));
const node = await this.vite.moduleGraph.getModuleByUrl(url);

const deps = new Set();
find_deps(node, deps);

const styles = new Set();

for (const dep of deps) {
const parsed = parse(dep.url);
const query = new URLSearchParams(parsed.query);

// TODO what about .scss files, etc?
if (
dep.file.endsWith('.css') ||
(query.has('svelte') && query.get('type') === 'style')
) {
try {
const mod = await this.vite.ssrLoadModule(dep.url);
styles.add(mod.default);
} catch {
// this can happen with dynamically imported modules, I think
// because the Vite module graph doesn't distinguish between
// static and dynamic imports? TODO investigate, submit fix
}
}
}

return {
module,
entry: `/${id}?import`,
css: [],
js: [],
styles: Array.from(styles)
};
},
manifest: this.manifest,
read: (file) => fs.readFileSync(path.join(this.config.kit.files.assets, file)),
root,
router: this.config.kit.router,
ssr: this.config.kit.ssr,
target: this.config.kit.target,
template: ({ head, body }) => {
let rendered = template
let rendered = fs
.readFileSync(this.config.kit.files.template, 'utf8')
.replace('%svelte.head%', () => head)
.replace('%svelte.body%', () => body);

Expand Down Expand Up @@ -213,69 +279,7 @@ class Watcher extends EventEmitter {
}

return rendered;
},
manifest: this.manifest,
load_component: async (id) => {
const url = path.resolve(this.cwd, id);

const module = /** @type {SSRComponent} */ (await this.vite.ssrLoadModule(url));
const node = await this.vite.moduleGraph.getModuleByUrl(url);

const deps = new Set();
find_deps(node, deps);

const styles = new Set();

for (const dep of deps) {
const parsed = parse(dep.url);
const query = new URLSearchParams(parsed.query);

// TODO what about .scss files, etc?
if (
dep.file.endsWith('.css') ||
(query.has('svelte') && query.get('type') === 'style')
) {
try {
const mod = await this.vite.ssrLoadModule(dep.url);
styles.add(mod.default);
} catch {
// this can happen with dynamically imported modules, I think
// because the Vite module graph doesn't distinguish between
// static and dynamic imports? TODO investigate, submit fix
}
}
}

return {
module,
entry: `/${id}?import`,
css: [],
js: [],
styles: Array.from(styles)
};
},
target: this.config.kit.target,
entry: '/.svelte/dev/runtime/internal/start.js',
css: [],
js: [],
dev: true,
amp: this.config.kit.amp,
root,
hooks: {
getContext: hooks.getContext || (() => ({})),
getSession: hooks.getSession || (() => ({})),
handle: hooks.handle || (({ request, render }) => render(request))
},
only_render_prerenderable_pages: false,
get_stack: (error) => {
this.vite.ssrFixStacktrace(error);
return error.stack;
},
get_static_file: (file) =>
fs.readFileSync(path.join(this.config.kit.files.assets, file)),
ssr: this.config.kit.ssr,
router: this.config.kit.router,
hydrate: this.config.kit.hydrate
}
}
);

Expand Down
Loading