Skip to content

Allocate unique reactTags for RN and Fabric - #12587

Merged
sebmarkbage merged 1 commit into
react:masterfrom
sebmarkbage:uniquereacttag
Apr 10, 2018
Merged

sebmarkbage merged 1 commit into
react:masterfrom
sebmarkbage:uniquereacttag

Conversation

@sebmarkbage

@sebmarkbage sebmarkbage commented Apr 10, 2018 •

Copy link
Copy Markdown
Contributor

We need these number sequences to not overlap and need some way for native to determine which one is which.

Took this opportunity to remove some abstract overhead.

In Fabric it is extra simple since they no longer overlap with root tags.

@sophiebits sophiebits left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why no shared module?

Took this opportunity to remove some abstract overhead.

In Fabric it is extra simple since they no longer overlap with root tags.
@sebmarkbage

Copy link
Copy Markdown
Contributor Author

We need to determine from native if it is Fabric or RN. It would require some kind of map storage to do that which is tricky since that would need to be managed by the GC. What is worse, is that we'd need to call into JS to figure this out.

@sophiebits

Copy link
Copy Markdown
Contributor

I mean with the same number structure, just centralized in one file. Makes it more obvious what needs changing next time.

@sebmarkbage

Copy link
Copy Markdown
Contributor Author

That requires some injection to set up the incremental parts. Unless I instead duplicate it in that file. Then once we remove one we'll never remove the other. I prefer to write like it would be if this was the future.

@sebmarkbage

Copy link
Copy Markdown
Contributor Author

I think at some point (soon), we'll do a code mod where React tag is actually an instance handle pointer (fiber) for Fabric.

@sebmarkbage

Copy link
Copy Markdown
Contributor Author

If we ever up this strategy, there are a bunch of places in native that needs to update too. Sprinkled across languages etc. So not sure the duplication here is that bad in context.

@sebmarkbage
sebmarkbage merged commit 2f7bca0 into react:master Apr 10, 2018
@sebmarkbage

Copy link
Copy Markdown
Contributor Author

cc @mdvacca

rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
Took this opportunity to remove some abstract overhead.

In Fabric it is extra simple since they no longer overlap with root tags.
mrizwanashiq pushed a commit to mrizwanashiq/react that referenced this pull request Jun 25, 2026
Took this opportunity to remove some abstract overhead.

In Fabric it is extra simple since they no longer overlap with root tags.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants