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

Allocate unique reactTags for RN and Fabric #12587

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Apr 10, 2018

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.

Copy link
Collaborator

@sophiebits sophiebits left a comment

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
Collaborator 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
Collaborator

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

@sebmarkbage
Copy link
Collaborator 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
Collaborator 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
Collaborator 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 facebook:master Apr 10, 2018
@sebmarkbage
Copy link
Collaborator Author

cc @mdvacca

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