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

refactor(runtime): make WeakMap obsolete #6156

Merged
merged 10 commits into from
Feb 13, 2025

Conversation

danielleroux
Copy link
Contributor

@danielleroux danielleroux commented Feb 12, 2025

What is the current behavior?

GitHub Issue Number: Fixes #6148

What is the new behavior?

Remove WeakMap and hold each HostRef on the actual DOM Element. Which makes the Weakmap itself not necessary anymore.

Issue to discuss:

  • Circular structure of HTMLElement (element -> $hostRef$ -> $hostElement$ -> $hostRef$ -> ...) Solved by using getHostRef
  • Add HostRef (containing vdom structure) to regular DOM (Performance ?) => Checked the performance haven't found any issues yet. Solved by using getHostRef
  • WebDriver.IO cannot convert element to JSON (Circular structure of HTMLElement) which results in failing tests atm. Solved by using getHostRef

Documentation

Does this introduce a breaking change?

  • Yes
  • No

Testing

Use existing tests, not added new ones.

@danielleroux danielleroux requested a review from a team as a code owner February 12, 2025 15:44
@christian-bromann
Copy link
Member

Circular structure of HTMLElement

Could we put it behind a simple getHostRef function to avoid this?

  • Add HostRef (containing vdom structure) to regular DOM (Performance ?)

Not sure about this. Again, if we put it behind a function it may just end up being a referenced stored in memory which I don't think has a big impact.

  • WebDriver.IO cannot convert element to JSON (Circular structure of HTMLElement) which results in failing tests atm.

I can take a look at it 😉

@johnjenkins thoughts?

@johnjenkins
Copy link
Contributor

Could we put it behind a simple getHostRef function to avoid this?

Sounds workable to me?
Would we need to make the method name more internal-stencil-y? To make sure we don't cause any name clashes?
e.g. something like __stencil__getHostRef() ?

@danielleroux
Copy link
Contributor Author

Use function call to resolve HostRef solves the circular structure 👍 Also the issue regarding webdriverio is fixed with the change.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Two minor nits

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Ok, I just double check the code, can we apply this as well in the following places?

  • src/hydrate/platform/index.ts
  • src/testing/platform/testing-constants.ts
  • src/testing/platform/testing-host-ref.ts
  • src/testing/platform/testing-platform.ts

@danielleroux
Copy link
Contributor Author

Ok, I just double check the code, can we apply this as well in the following places?

  • src/hydrate/platform/index.ts
  • src/testing/platform/testing-constants.ts
  • src/testing/platform/testing-host-ref.ts
  • src/testing/platform/testing-platform.ts

Sure! Makes sense to remove the weakmaps there too.

hostRef.$lazyInstance$ = lazyInstance;
return hostRefs.set(lazyInstance, hostRef);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the return value as weakmap is somewhere used, i tried to check the code but couldnt find any references which depends on the return value.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it doesn't have any impact.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Thank you so much, this is awesome 👍

@christian-bromann christian-bromann merged commit 4add75c into ionic-team:main Feb 13, 2025
70 of 71 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.

bug: regression for disconnected components
3 participants