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

fix: Improve prerender errors #1756

Merged
merged 4 commits into from Dec 18, 2022
Merged

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Dec 17, 2022

What kind of change does this PR introduce?

Bugfix / revert

Did you add tests for your changes?

N/A

Summary

TLDR:

Before PR Improved Error Logic Improved Error Logic & Disabled SSR Build Minification
Before this PR With Error Logic Altered Disabled SSR Build Minification

Partial revert of #1426

Having used it for a bit, I don't find it to be particularly compelling, especially w/ Webpack v5 -- methodName is often null and not correct enough for determining user vs lib code.

Additionally, I've found the stack trace locations (from Node / v8, to be clear) to be exceptionally poor on Webpack's minimized output, and so have elected to not minimize our SSR build.

Take the following:

src/routes/home/index.js

import { h } from 'preact';
import style from './style.css';

console.log(foo.bar);

const Home = () => (
	<div class={style.home}>
		<h1>Home</h1>
		<p>This is the Home component.</p>
	</div>
);

export default Home;

As foo is not defined, we should get a (reference) error here (and we do). However, the stack trace of ssr-bundle.js actually points to the h("div", { class: ... }, ...).

Why this is, I don't know. Mix of Webpack's output and Node maybe? Regardless, it's not great and reduces the quality of our error reporting (as it highlights the wrong line). But what we can do is just not minimize the SSR output. As it's only ran in Node, we don't have any particular reason to minimize, and a couple against, such as:

  • faster builds of the SSR bundles as minification takes time
  • better error positions if prerender errors occur

Does this PR introduce a breaking change?

No

@rschristian rschristian requested a review from a team as a code owner December 17, 2022 23:53
@changeset-bot
Copy link

changeset-bot bot commented Dec 17, 2022

🦋 Changeset detected

Latest commit: c249355

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

let color = i === 0 ? red : yellow;
let line = position.line + i;
let sourceLine = sourceLines[line - 1];
sourceCodeHighlight += sourceLine ? `${color(sourceLine)}\n` : '\n';
Copy link
Member Author

Choose a reason for hiding this comment

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

Rather importantly, we weren't handling blank lines correctly as the ternary was falling back to '', instead of the new line as it does above. This caused further issues in line positioning.

@rschristian rschristian force-pushed the feature/improve-prerender-errors branch from cb442c2 to 60e562d Compare December 18, 2022 00:12
@rschristian rschristian changed the title fix: Poor prerender errors fix: Improve prerender errors Dec 18, 2022
@rschristian rschristian force-pushed the feature/improve-prerender-errors branch from c144bc5 to c249355 Compare December 18, 2022 00:30
@rschristian rschristian merged commit a41d498 into master Dec 18, 2022
@rschristian rschristian deleted the feature/improve-prerender-errors branch December 18, 2022 00:36
@preact-bot preact-bot mentioned this pull request Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant