-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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(react): IonNav apply properties to page components #25603
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update the docs for rootParams
as well?
I have a thought here on how to support both implementation patterns... will report back if it works. |
Ok this is ready, we can support both memorized components and |
|
||
Component.props = propsOrDataObj; | ||
refMap.set(component, hostComponent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One instance of the delegate is constructed for one instance of IonNav
, but IonNav
can attach multiple views to the DOM.
The map allows us to keep track of all instances of the individual views that were pushed inside the Nav. We can then do a look-up against the map to find the correct instance to remove the view from the DOM.
In the previous implementation, the value of Component
would be reset when a new view was pushed, which prevented the previous view from ever being removed.
Alternatively you could use an object instead of a map here, but since we are associating to a React component, a WeakMap
would be my recommendation here so that it does not interfere with garbage collection.
Pull request checklist
Please check if your PR fulfills the following requirements:
ionic-docs
repo, in a separate PR. See the contributing guide for details.npm run build
) was run locally and any changes were pushednpm run lint
) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Root params set on
IonNav
are not inherited by the root component.Issue URL: resolves #25602
What is the new behavior?
IonNav
will now assign the properties to the page components correctly.IonNav now supports two ways to assign properties to page components:
Memorized Components
rootParams
Additionally, this PR caches element references, which partially resolves an issue with Vite + React and hmr clearing the views within IonNav.
Does this introduce a breaking change?
Other information
Dev-build:
6.1.15-dev.11657908598.17be2d80