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 warning when calling VirtualDom new with props #3911

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ealmloff
Copy link
Member

Fixes #3872

@ealmloff ealmloff added bug Something isn't working core relating to the core implementation of the virtualdom labels Mar 25, 2025
@ealmloff ealmloff requested a review from a team as a code owner March 25, 2025 21:23
@jkelleyrtp
Copy link
Member

I get many many warnings on random dioxus examples - I think our warning system is rather underbaked.

I think we should remove it (the warnings integration) for the time being. IMO when we add it back it maybe shouldn't be using stdout and instead log over the devtools or something.

This PR is a good example of a long list of workarounds we're going to need to add and we've already added quite a few.

@DogeDark
Copy link
Member

DogeDark commented Apr 9, 2025

I mostly agree. I can see a warning system being useful in certain situations, but I often find myself chasing down a warning when the implementation works fine.

Most of the warnings I get are signal-based (write during run, write in component body). Maybe a different implementation that detects infinite write-read would be better instead of assuming all signal writes during these states are an issue.

@ealmloff
Copy link
Member Author

I agree, the signal to noise ratio of warnings need to be higher. I think that is more due do the nature of the things we are trying to warn about than the system itself. If it was the system, I don't think removing and eventually rewriting it would make it more reliable.

Currently I think we have 4 different warnings:

  1. Calling a component like a function: this works fine unless you use components as function pointers which can cause some issues internally (like this PR). IIRC you should never use components as function pointers in user code because it breaks the TypeId component diffing
  2. Writing in the body of a component: breaks if you only conditionally write in the body of a component like use_reactive
  3. Writing in the scope you read in: Again breaks if you only conditionally write. We now also just don't rerun if you write in a scope the signal is subscribed to. That behavior may be confusing, but it is less catastrophic than looping forever
  4. Reading a hoisted copy value: I don't remember running into issues with this warning?

We should probably remove 2 and 3. I think 4 has been pretty reliable so it might be useful to keep

@DogeDark
Copy link
Member

Yeah, 2 and 3 are the most common. I don't think I've ever seen 4.

We now also just don't rerun if you write in a scope the signal is subscribed to.

Is that behavior in 0.6.3?

@ealmloff
Copy link
Member Author

Is that behavior in 0.6.3?

Yes, I think it was added before the 0.6 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core relating to the core implementation of the virtualdom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An error gets printed when calling VirtualDom::new_with_props
3 participants