8000 Remove the mutation observer feature by stephencookdev · Pull Request #100 · reactive-elements/reactive-elements · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@stephencookdev
Copy link
Contributor

The mutation observer idea is cool, but largely unnecessary - there shouldn't be much dynamism on the web-component side of your web-component/React bridge. The dynamic stuff should really all be living on the React side of stuff.

But more specifically, the code is a little buggy as things stand; when a component re-mounts, the code to determine the element's children does not calculate them correctly (possibly just if the remount is near the initial mount?), and instead determines the children array to always be []

So it seems that the easiest fix is probably just to remove it entirely

@jackfranklin
Copy link
Collaborator

@stephencookdev I had one thought about this today. We do have some situations where we've rendered:

<some-react-thing x="1" />

And then something updates it to be:

<some-react-thing x="2">

We do need to make sure we deal with that case. I wonder if when you define a Reactive Element if you should give us a list of props that will change, and then we can observe and listen to changes using the native spec: https://developers.google.com/web/fundamentals/web-components/customelements#reactions - happy for this to be a separate PR.

@ChristianMurphy does removing this feature cause problems for you?

@stephencookdev
Copy link
Contributor Author

@stephencookdev I had one thought about this today. We do have some situations where we've rendered:

<some-react-thing x="1" />

And then something updates it to be:

<some-react-thing x="2">

Why + where do we do that? Just trying to understand it as a feature fully

@ChristianMurphy
Copy link
Member

does removing this feature cause problems for you?

Major issues, attribute updates and slot/children updates are being used.

Why + where do we do that?

HTML attributes are the equivalent of React props.
Anytime component state needs to be updated by it's parent, this is needed.

@PixelsCommander
Copy link
Collaborator
PixelsCommander commented Feb 14, 2019 via email

@ChristianMurphy
Copy link
Member
ChristianMurphy commented Feb 14, 2019

we can observe and listen to changes using the native spec: https://developers.google.com/web/fundamentals/web-components/customelements#reactions

I remember there is an event named attributesChanged on a custom element

A limitation of the spec is that attributes that will change need to be explicitly defined in static get observedAttributes()
Defining that in the application could be an option for replacing the attributes part of the mutation observer, but not the childList or subtree for slot handling.

It would also make the API a bit clunky.
React components are often paired with prop-types, needing to describe the properties and their types, then list them again to get the attributes to be watched, would be awkward.

Ideally prop-types would be used to define static get observedAttributes() and could replace the implicit type coercion in:
https://gi 8000 thub.com/PixelsCommander/ReactiveElements/blob/2e966731df07832b421480ef445baa4154411f89/src/utils.js#L87-L119
with explicit typing.

@stephencookdev
Copy link
Contributor Author

Okay, I'm going to try messing about with this a bit to try and get the propagation of HTML attributes --> React prop updates working, without causing remounting bugs.

@jackfranklin
Copy link
Collaborator

Why + where do we do that? Just trying to understand it as a feature fully

We use it where we have Elm rendering a custom web element - rather than re-render from scratch, if props update Elm will just update the attributes in place, so we need to deal with that. For our use case just using observedAttributes would work fine, but clearly not for @ChristianMurphy and their usage.

Happy to chat through or have a think about this in person.

@stephencookdev stephencookdev mentioned this pull request Mar 4, 2019
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.

4 participants

0