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

feat: log a warning instead of throwing an error for server host mismatch error #7236

Open
wants to merge 1 commit into
base: build/v2
Choose a base branch
from

Conversation

Varixo
Copy link
Member

@Varixo Varixo commented Jan 9, 2025

Log a warning instead of throwing an error for server host mismatch error

@Varixo Varixo self-assigned this Jan 9, 2025
@Varixo Varixo requested a review from a team as a code owner January 9, 2025 16:21
Copy link

changeset-bot bot commented Jan 9, 2025

🦋 Changeset detected

Latest commit: 8a2a90e

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

This PR includes changesets to release 5 packages
Name Type
@qwik.dev/core Patch
eslint-plugin-qwik Patch
@qwik.dev/react Patch
@qwik.dev/router Patch
create-qwik 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

@Varixo Varixo marked this pull request as draft January 9, 2025 16:40
@Varixo Varixo force-pushed the v2-warn-server-mismatch-error branch from 8387e03 to 8a2a90e Compare January 9, 2025 19:22
Copy link

pkg-pr-new bot commented Jan 9, 2025

Open in Stackblitz

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@7236
npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/router@7236
npm i https://pkg.pr.new/QwikDev/qwik/eslint-plugin-qwik@7236
npm i https://pkg.pr.new/QwikDev/qwik/create-qwik@7236

commit: 8a2a90e

Copy link
Contributor

github-actions bot commented Jan 9, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
qwik-docs ✅ Ready (View Log) Visit Preview 8a2a90e

@Varixo Varixo marked this pull request as ready for review January 9, 2025 19:39
@@ -50,7 +50,6 @@ export const codeToText = (code: number, ...parts: any[]): string => {
"Element must have 'q:container' attribute.", // 42
'Unknown vnode type {{0}}.', // 43
'Materialize error: missing element: {{0}} {{1}} {{2}}', // 44
'SsrError: {{0}}', // 45
Copy link
Member

Choose a reason for hiding this comment

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

You can't just delete a line here, each error must be on a single line so that error links work.

If you renumber all errors it's ok but once it's stable you shouldn't renumber any more. What you can do is use old numbers for new errors I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I know, it is alpha thats why I removed the error

logWarn(errorMessage);
return null;
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

this makes sorting unstable. If you don't see a difference, return 0.

Copy link
Member

Choose a reason for hiding this comment

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

actually shouldn't we still sort by host depth?

and the warning should happen when ssrnodes that already streamed are marked dirty, not when tasks rerun, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

for some reason returning 0 is not working, I will look at it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting For Review
Development

Successfully merging this pull request may close these issues.

2 participants