Skip to content
This repository has been archived by the owner on Jan 29, 2023. It is now read-only.

Remove all usage of IDs #78

Merged
merged 8 commits into from
Oct 10, 2018
Merged

Remove all usage of IDs #78

merged 8 commits into from
Oct 10, 2018

Conversation

SachsKaylee
Copy link
Collaborator

@SachsKaylee SachsKaylee commented Oct 9, 2018

Related to #62 (last paragraph)
Possibly related to #75

This component currently makes heavy usage of DOM IDs. Almost every node created gets its own ID. This makes it very easy to fetch a specific DOM node using the document.getElementById function.

This, however, comes at a price. Since IDs are global they are very slow to add and remove. If we have lots of them(like we do) this will have a measurable performance impact.
Furthermore due to their global nature, the risk of clashes is real. We currently try to avoid clashes to the best of our abilities by prefixing all IDs with a random GUID. This however, makes rehydration of server side rendered content nearly impossible, since the client and the server will both generate a different GUID unless they use the same random number seed.

Since we do not actually use the IDs for anything we can easily remove them and gain a measurable speed boost as well as enable SSR.

I also suspect that this will allow us to simplify our unit testing setup a bit, but I'm still looking into that.


Important note: This could be a breaking change if we consider the DOM values to be part of our API.

@SachsKaylee
Copy link
Collaborator Author

Yeah, we could get of some unit testing scaffolding, but that's a different PR.

@ghost ghost mentioned this pull request Oct 10, 2018
@AndrewRedican
Copy link
Owner

AndrewRedican commented Oct 10, 2018

@PatrickSachs,

I just went through the changes and coments, and I think this is brilliant.

The previous code prevented an almost impossible challenge for SSR testing. I see how you used createRef() to provide a solution to a few places the component internally needed a reference to an element without but using an id. Quite frankly I didn't know about it until now. I need to update myself now with the latest React documentation.

This looks great.


A few comments:

  1. We should probably get rid of the randomString() function altogether, it was use especifically for using ids. If there is no need for ids, we can go ahead and remove randomString().

  2. We should probably keep the id property as an opt-in feature to provide the means for a developer to set an id attribute to the container elements or elements of this component, with the exception of tokens. If an id property is not provided elements should not be assigned any id attribute at all. Let me know what you think.

  3. In src/index.js I would like to understand changes done on lines 570 through 576 and lines 603 though 608.


Code looks good but I haven't had the change to test this. I will get back to you today.

@ghost
Copy link

ghost commented Oct 10, 2018

Hey,

thank you very much.
I’ll make the necessary commits once I get home this evening.

I see how you used createRef() to provide a solution to a few places the component internally needed a reference to an element without but using an id.

I wasn't sure if the createRef() call is even required. The main work is being done by the callback in the "ref" prop of the respective elements.

However, as @ntfs32 pointed out in #75 "React.createRef" is a new API function in React 16, that did not exist in React 15. While we explicitly only support React 16 and up, I think we could be able to maintain backward compatibility with a small workaround.

We should probably get rid of the randomString() function altogether, it was use especifically for using ids. If there is no need for ids, we can go ahead and remove randomString().

Good catch. I agree.

We should probably keep the id property as an opt-in feature to provide the means for a developer to set an id attribute to the container elements or elements of this component, with the exception of tokens. If an id property is not provided elements should not be assigned any id attribute at all. Let me know what you think.

I don't see a reason against this. This could prove useful if people e.g. want to do styling with standard CSS.

In src/index.js I would like to understand changes done on lines 570 through 576 and lines 603 though 608.

Essentially, the same element had two „onPaste“ listeners attached to it. One using the React onPaste callback, and the other one attached using addEventListener(‚paste’).

While having two event listeners on one element isn’t bad, it is slightly confusing in regards to ordering and why one was attached using React and the other one directly through the DOM.

I essentially just went ahead and combined the logic of both event handlers to only use the React onPaste callback. This seemed to work out in my tests, but more testing is always appreciated.

@SachsKaylee
Copy link
Collaborator Author

SachsKaylee commented Oct 10, 2018

We don't actually need the createRefs. The createRefs and callback ref functions are separate systems.

https://blog.logrocket.com/how-to-use-react-createref-ea014ad09dba

Copy link
Owner

@AndrewRedican AndrewRedican left a comment

Choose a reason for hiding this comment

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

Hi @PetrykowskiM,

I tested the changes. This is good to go.

@AndrewRedican AndrewRedican merged commit c4e4979 into master Oct 10, 2018
@AndrewRedican AndrewRedican deleted the no-ids branch October 10, 2018 21:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants