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

updating the informations needed when adding a root node to a tree in react-devtools overview doc #19979

Merged
merged 1 commit into from Oct 8, 2020

Conversation

IDrissAitHafid
Copy link
Contributor

Summary

  1. Updated the value of ElementTypeRoot to 11, as it is here:

    export const ElementTypeRoot = 11;

  2. Added a fifth number required when adding a root node to a tree, as I found and understand from this line:

    pushOperation(hasOwnerMetadata ? 1 : 0);

    and from this one:
    const hasOwnerMetadata = operations[i] > 0;

cc @bvaughn

@facebook-github-bot
Copy link

Hi @IDrissAitHafid!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 8, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b4a7858:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Oct 8, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against b4a7858

@sizebot
Copy link

sizebot commented Oct 8, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against b4a7858

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@bvaughn bvaughn merged commit 6eca8ef into facebook:master Oct 8, 2020
@IDrissAitHafid
Copy link
Contributor Author

Question: shouldn't this line be deleted from the example of adding a leaf node?
3, // encoded display name size
`
At least that's what I understood from reading the code, but this phrase confused me:

Adding a leaf node takes a variable number of numbers

So, I just wanted to ask for your confirmation before opening a PR 😅.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 8, 2020

I have not made much of an effort to keep the OVERVIEW doc up to date as DevTools has changed over time. I think it's worth doing, it's just something I haven't had/taken/made time for.

WRT the "encoded display name size" – we used to send encoded strings inline, so that position carried the size of the string and then the next N numbers were the actual encoded string. At some point, we migrated things to use a string table and so now we just send an ID. That means the overall number of elements is fixed now, where as it used to be variable. Looks like the attempt to update the OVERVIEW after this change was flubbed a bit.

So the "encoded display name size" line could be deleted and the "variable number" comment could be updated.

@IDrissAitHafid
Copy link
Contributor Author

Thanks for the clarification.
Anyway, the code itself is well documented and it's just another chance to contribute 😄

koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
… react-devtools overview doc (facebook#19979)

Co-authored-by: Idriss AITHAFID <Idriss.AITHAFID@um6p.ma>
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.

None yet

5 participants