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(dev): Reports what is happening when SSR dirty tasks in taskStaging. #6642

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

genki
Copy link
Contributor

@genki genki commented Jul 4, 2024

Overview

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

When I have been developing Qwik app, the line came out in stderr.

task Nav_component_useTask_9gk0OXCxvU0

It was very hard to know what was happening and which module printed out it from where.
You know, the dev tool of the browser kindly adds the call stack to the console.error but at the server side, it's wild :)

Use cases and why

Finally I found the console.error that printed out, then I have added more information about what is happening.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@genki genki requested review from a team as code owners July 4, 2024 08:34
Copy link

netlify bot commented Jul 4, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 50bf471

Copy link

pkg-pr-new bot commented Jul 4, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

commit: 50bf471

@builder.io/qwik

npm i https://pkg.pr.new/@builder.io/qwik@6642

@builder.io/qwik-city

npm i https://pkg.pr.new/@builder.io/qwik-city@6642

eslint-plugin-qwik

npm i https://pkg.pr.new/eslint-plugin-qwik@6642

create-qwik

npm i https://pkg.pr.new/create-qwik@6642


templates

@genki
Copy link
Contributor Author

genki commented Jul 4, 2024

BTW, this console.error is really needed?
My app seemed work without problem even if this error has reported.
I felt the line below handles the task

taskPromises.push(maybeThen(task.$qrl$.$resolveLazy$(containerEl), () => task));

@genki
Copy link
Contributor Author

genki commented Jul 4, 2024

I have digged about this issue, and found #5741 has been already solved the problem about the SSR dirty tasks.
@wmertens Why you left the console.error?

@wmertens
Copy link
Member

wmertens commented Jul 4, 2024

Can you also run api.update so those api changes are gone?

I don't think I added those logs but they do show something that should not happen I guess.

In any case in V2 this code is completely different so it doesn't matter much.

@genki
Copy link
Contributor Author

genki commented Jul 4, 2024

@wmertens I'm a bit confusing. I tried the api.update but it didn't change anything.
I meant the L313 of this patch "packages/qwik/src/core/render/dom/notify-render.ts" you wrote.
I wanted know why you added this console.error, for ex, because of something are not solved yet so there had been needed to show caution?
If not so, can I simply remove this console.error instead of fixing its message richer.

@genki
Copy link
Contributor Author

genki commented Jul 4, 2024

@wmertens I realized I had mistakenly run pnpm run api.update --api.
I have fixed it.

@wmertens
Copy link
Member

wmertens commented Jul 9, 2024

Right that looks like I added it as a debug tool and I didn't see it because it was .error instead of .log. You can simply remove the line in this PR.

@genki
Copy link
Contributor Author

genki commented Jul 9, 2024

I did it :)

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

LGTM

@wmertens wmertens merged commit 281de97 into QwikDev:main Jul 9, 2024
20 checks passed
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

2 participants