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

Add use_on_mount and fix some typos #1497

Merged
merged 5 commits into from Oct 4, 2023
Merged

Conversation

tigerros
Copy link
Contributor

@tigerros tigerros commented Sep 26, 2023

  • Adds a new use_on_mount hook. Works like use_effect but with no dependencies.
  • Adds a little extra documentation to use_effect.
  • Fixes some typos.

There's some discussion about if use_on_mount should instead be use_on_created or use_on_component_mount because of potential confusion with the onmounted event. This is what I prefer, but I guess it's still up for discussion.

@ealmloff
Copy link
Member

There's some discussion about if use_on_mount should instead be use_on_created or use_on_component_mount because of potential confusion with the onmounted event. This is what I prefer, but I guess it's still up for discussion.

Copying over some of the discussion from discord for context:

  • This hook wouldn't always run after the component is mounted to the dom. It could return None, or be suspended. Because of that some javascript operation that need some element to be mounted would fail unlike the onmounted event
  • This naming is similar to use_onunmount (which mirrors this functionality), but also onmount which does something different

@tigerros
Copy link
Contributor Author

  • This hook wouldn't always run after the component is mounted to the dom. It could return None, or be suspended. Because of that some javascript operation that need some element to be mounted would fail unlike the onmounted event
  • This naming is similar to use_onunmount (which mirrors this functionality), but also onmount which does something different

Yeah, I saw use_on_unmount and thought use_on_mount makes sense. But the first point is also true, so I really don't know. However, if we're not going to use "mount", then I think we should also rename use_on_unmount. Make a suggestion and I'll commit it.

@ealmloff
Copy link
Member

Yeah, I saw use_on_unmount and thought use_on_mount makes sense. But the first point is also true, so I really don't know. However, if we're not going to use "mount", then I think we should also rename use_on_unmount. Make a suggestion and I'll commit it.

Completely agree, we should keep the component mount and unmount names similar. I like use_on_create and use_on_destroy because it feels different from the component being mounted to the DOM. But use_on_initialized, and use_on_removed or use_after_first_run and use_before_remove also work. Anything that is clearly different from the element event and is short works. It could also be a good idea to leave this PR open for a bit to see if anyone else has some naming ideas

@tigerros
Copy link
Contributor Author

I updated it to be use_on_create and use_on_destroy. The problem is that this is now a breaking change.

@ealmloff ealmloff added the breaking This is a breaking change label Sep 26, 2023
@tigerros
Copy link
Contributor Author

tigerros commented Sep 26, 2023

I added use_on_unmount back, but made it deprecated. This might be the best solution, but I'm not sure if it isn't better to just remove it entirely.

@ealmloff ealmloff added enhancement New feature or request hooks Changes to built-in hook package and removed breaking This is a breaking change labels Sep 27, 2023
@ealmloff ealmloff merged commit 30a3283 into DioxusLabs:master Oct 4, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hooks Changes to built-in hook package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants