Generate SimpleEventPlugin data structures at runtime #7616
Conversation
ghost
added
|
I don't like code that fails the grep test. :\ http://jamie-wong.com/2013/07/12/grep-test/ |
|
@spicyj yeah, it's not ideal. I really don't understand why we have such a convoluted setup of layers of indirections with repeated things to setup event names. |
|
I think the benefit here outweighs the possibility that somebody (us?) would be grepping |
| captured: 'onLoadStartCapture', | ||
| }, | ||
| }, | ||
| // Note: We do not allow listening to mouseOver events. Instead, use the |
There was a problem hiding this comment.
Why was this comment here? I don’t understand what it refers to because mouseOver is defined just below. Is this still relevant and should some version of this be copied to new code?
There was a problem hiding this comment.
Must be outdated. We used to not have onMouseOver/onMouseOut but now we do.
There was a problem hiding this comment.
I thought I ported it back but it slept through the several revisions I've done, oops. Good to know it is no longer needed.
Maybe we should address that and not just try to trim a few bytes off of code that not many understand fully? |
|
Why not do one step at a time? IMO it would be easier to address when there is less code because it seems easier to me to change data structures in one place rather than all over the file. |
|
Okay. FWIW if anyone wants to understand how the current events work, I am happy to explain it in depth. |
|
I want to. Can we also record this for future generations? cc @kentcdodds |
|
I would also love to know how it works! |
|
As for the grep test, I wonder if you could just add the things you want to grep for as comments at the top of the file with a big comment explaining what's going on in the file (and why it's better to be this way). |
|
My concern with meta-programming in general is different. It is crazy difficult to unwind this when you have special cases and encourages you to put everything in the same kind of configuration system even though the particular thing you're trying to fix is a new case that doesn't fit. That's true before and after this change. I'd rather explode these into the closest point where they're needed to be checked. For example, does it really make sense for all of these to bubble and therefore require every type of element to be able to handle any event we have? Leading to special cases like all images needing |
|
I don’t feel strongly about collapsing everything here. The thing that seemed odd to me originally when I suggested this is that I can’t imagine us wanting a |
|
We'll probably need to rethink this strategy regardless because of the explosion of combinatorial options now: #6436 |
|
Yes but this will take a while and is not a priority. |
|
I'll bow out because there is too much brainpower on this PR already. Either way is fine. I'll just leave it that I don't think this is a good systemic strategy to keep doing. We shouldn't avoid making the hard decisions by making existing code shorter in clever ways and more difficult break apart later when we need to make the hard decisions. We don't even know if this is a net win because this might cause longer module init time, might cause the string ropes to not be interned or whatever. There's more impactful work to do in this space such as just getting rid of the Capture events and solving the Passive listener case, or getting rid of most of this list by bypassing our event system and passing them through. |
We used to copy and paste the same big blob many times in order for it to work with keyOf which is no longer a constraint. This pull request takes a list of all the events as string and generate those data structures at runtime. It reduces the size of React by 1k post gzip and flow is able to extract the structure out of it :)
vjeux
force-pushed
the
runtime_SimpleEventPlugin
branch
from
5d9cd3e
to
c0b4659
Compare
|
Fixed the lint issue, the build should now be green. I'd love a final call here, either an accept or that I should drop this pull request :) |
|
Didn't we agree yesterday that it was fine? |
|
(Accepted for the historical record.) |
We used to copy and paste the same big blob many times in order for it to work with keyOf which is no longer a constraint. This pull request takes a list of all the events as string and generate those data structures at runtime. It reduces the size of React by 1k post gzip and flow is able to extract the structure out of it :)
We used to copy and paste the same big blob many times in order for it to work with keyOf which is no longer a constraint. This pull request takes a list of all the events as string and generate those data structures at runtime. It reduces the size of React by 1k post gzip and flow is able to extract the structure out of it :) (cherry picked from commit 1229a23)
This undoes facebook#7616
We used to copy and paste the same big blob many times in order for it to work with keyOf which is no longer a constraint. This pull request takes a list of all the events as string and generate those data structures at runtime.
It reduces the size of React by 1k post gzip and flow is able to extract the structure out of it :)