Temporary workaround for use of purged ID in updateComponent

[syranide]

Would this be an acceptable temporary workaround for #1569 @sebmarkbage? Seeing as the alternative seems to be quite an extensive refactor, which may not come any time soon.

With this PR and #1568 (which is straightforward) it would be possible to go ahead with my #1570 PR (remove reactid from DOM) when you guys have reviewed it and decided whether you prefer lazy/immediate traversal.

[sebmarkbage]

I think the idea of dangerouslyReplaceNodeWithMarkupByID is that it can be serialized and sent over the wire or have different implementations. Not sure a callback is the best API. There is something else that is messy here. Will look into this again...

[syranide]

Yeah, I don't think callback is great either (but it didn't break the core/browser separation at least :)).

So at current it looks like:

1. Get prevComponent.ID
2. Unmount prevComponent (a) + purge prevComponent.ID (b)
3. Instantiate nextComponent
4. Mount nextComponent (c) + create nextComponent.markup (d)
5. Replace node for prevComponent.ID with nextComponent.markup

Step 5 is invalid, step 2-4 as-is cannot be reordered, step 5 depends on step 4 and step 1 must be before step 2. So as-is, there is nothing we can do, as far as I can tell.

Solutions?

1. Replace prevComponent.ID with a "target locator" (say a DOM node) or similar feature (ugh).
2. Separate unmounting and purging (steps: 1, 2a, 3, 4, 5, 2b).
3. Separate creation of markup and mounting (steps: 1, 2a, 3, 4d, 5, 2b, 4c).
4. ???

Solution 2 seems like the least invasive and convoluted, as it simply requires the addition of another method and moving the purging there. However, theoretically, it also means that there would have to be a similar separation for mounting if we choose to go with immediate traversal. But even then, it seems like a surprisingly natural separation to me.

Perhaps you have greater plans in mind, but if this sounds reasonable to you I could cobble up a PR for it.

[sebmarkbage]

Solution 2 sounds great and is also in line with my greater plans. :)

On Jul 24, 2014, at 2:38 PM, Andreas Svensson [email protected] wrote:

Yeah, I don't think callback is great either (but it didn't break the core/browser separation at least :)).

So at current it looks like:

1. Get prevComponent.ID
2. Unmount prevComponent (a) + purge prevComponent.ID (b)
3. Instantiate nextComponent
4. Mount nextComponent (c) + create nextComponent.markup (d)
5. Replace node for prevComponent.ID with nextComponent.markup

Step 5 is invalid, step 2-4 as-is cannot be reordered, step 5 depends on step 4 and step 1 must be before step 2. So as-is, there is nothing we can do, as far as I can tell.

Solutions?

1. Replace prevComponent.ID with a "target locator" (say a DOM node) or similar feature.
2. Separate unmounting and purging (steps: 1, 2a, 3, 4, 5, 2b).
3. Separate creation of markup and mounting (steps: 1, 2a, 3, 4d, 5, 2b, 4c).
4. ???

Solution 2 seems like the least invasive and convoluted, as it simply requires the addition of another method and moving the purging there. However, theoretically, it would also mean that there would also have to be a similar separation for mounting, e.g. if we choose to go with immediate traversal. But even then, it seems as a surprisingly natural separation to me.

Perhaps you have greater plans in-mind, but if this sounds reasonable I could cobble up a PR for it.

—
Reply to this email directly or view it on GitHub.

[syranide]

Hmm, I tried implementing #2 and it's definitely no problem, but it doesn't fit as neatly as I would've expected. It seems to me that there must be a better solution. The fact that updating a component in-place works just fine, but replacing it is a headache. So instinctively, it feels wrong to complicate the API and introduce more traversals (and a weird in-between state) for "just" a single operation.

It also seems like an odd implementation right now as dangerouslyReplaceNodeWithMarkupByID is executed in updateComponent, as opposed to in a transaction like mountComponent is. If we add unmountComponent (or replaceComponent with "null" for unmount perhaps?) to the transactions as well, it seems like it could have a lot benefits.

It would follow naturally to just add a purgeComponent to the transactions and we get the separation we require (alternatively, reusing the unmount/replaceComponent CallbackQueue but adding an argument), so other end-targets don't have to implement this if they don't need it (same goes for the opposite of purgeComponent, which we may or may not want depending on immediate/lazy traversal).

We use _mountIndex to coordinate mountComponent inside transactions, so it makes sense that we could do it for replaceComponent as well, for knowing where to insert/replace. All problems solved without really touching core... I think?

Summary: extend ReactReconcileTransaction instead with necessary separation, rather than the ReactComponent-interface.

Reasonable or completely off the rails? :)

[sebmarkbage]

A lot of this touches refactoring that we're planning to do anyway. For example staging all updates on a a queue instead of immediately updating. Also the replacing step will move out of composite so that it's tied to the runtime environment where it was updated rather than hardcoded. It also may affect how fragments can be implemented.

On Jul 25, 2014, at 3:59 AM, Andreas Svensson [email protected] wrote:

Hmm, I tried implementing #2 and it's definitely no problem, but it doesn't fit as neatly as I would've expected. It seems to me that there must be a better solution. The fact that updating a component in-place works just fine, but replacing it is a headache. So instinctively, it feels wrong to complicate the API and introduce more traversals (and a weird in-between state) for "just" a single operation.

It also seems like an odd implementation right now as dangerouslyReplaceNodeWithMarkupByID is executed in updateComponent, as opposed to in a transaction like mountComponent is. If we add unmountComponent (or replaceComponent with "null" for unmount perhaps?) to the transactions as well, it seems like it could have a lot benefits.

It would follow naturally to just add a purgeComponent to the transactions and we get the separation we require (alternatively, reusing the unmount/replaceComponent CallbackQueue but adding an argument), so other end-targets don't have to implement this if they don't need it.

We use _mountIndex to coordinate mountComponent inside transactions, so it makes sense that we could do it for replaceComponent as well, for knowing where to insert/replace. All problems solved without really touching core... I think?

Summary: extend ReactReconcileTransaction instead with necessary separation, rather than the ReactComponent-interface.

Reasonable or completely off the rails? :)

—
Reply to this email directly or view it on GitHub.

[syranide]

@sebmarkbage Ah, that sounds like something I should leave up to you guys then. :)

[syranide]

@sebmarkbage Just a curious follow-up question, you don't see any issues with doing away with the reactids in the DOM (and the concatenated reactids completely), like #1570, right?

[sebmarkbage]

I think that we can definitely get rid of the concatenated IDs. In fact, we really should since long keys can be the best way to model an element sometimes. It's also architecturally important that the keys are part of a different layer than the DOM interaction.

Yea I don't see any issues with it. I wouldn't go as far as commit to never putting IDs in the DOM. It might be beneficial to have a numeric IDs in the HTML generation depending on what strategy is most important at the time. But we should definitely have the option to do both. I'd even be open to maintain multiple strategies in the same code base and test which one works best. Maybe even switch depending on use case and browser.

[syranide]

@sebmarkbage Great, yeah from my tests (unless you have other goals in mind), removing the IDs from the DOM is rather consistently positive (each attribute is surprisingly costly). The only downside to keeping the lazy approach is that; without a marker (i.e, data-react), we must always traverse up to the nearest React element or the document (for events!).

However, if we move the listeners away from the document and onto React roots instead, this would be a non-issue (and I don't think it's an issue regardless). Or if we choose to go with the instant traversal (which is also consistently faster than what we have today) that is a non-issue completely as we know exactly which nodes are owned by React.

Anyway, for evaluation purposes, it would be very easy to have all 3 strategies available. Lazy traversal without reactid, immediate traversal without reactid and lazy traversal with reactid (but reactid is pointless really and all we need is a data-react marker). They're based on the same code and there are only minor differences between them. We wouldn't be able to explore the benefits of immediate traversal, but it would be a great way to determine if the results point conclusively one way or the other.

But I'll wait for you guys to get around to the refactor, unless you're willing to go with (something like) this PR in the mean time, and then I'll polish up my PR #1570.