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

Attempting to load Redux devtools freezes the page #919

Closed
nylen opened this issue May 26, 2017 · 8 comments
Closed

Attempting to load Redux devtools freezes the page #919

nylen opened this issue May 26, 2017 · 8 comments
Assignees
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Bug An existing feature does not function as intended

Comments

@nylen
Copy link
Member

nylen commented May 26, 2017

Self explanatory I think. We should git bisect to figure out when this started happening.

@nylen nylen added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Bug An existing feature does not function as intended labels May 26, 2017
@BE-Webdesign
Copy link
Contributor

BE-Webdesign commented May 26, 2017

I think the dev tools are running out of memory trying to parse and stringify the large body of text. If you cut the post content down to a smaller size the dev tools will run. This kind of has greater implications as well.

@nylen
Copy link
Member Author

nylen commented May 27, 2017

Nice find @BE-Webdesign, I can confirm that shortening post-content.js to only a single text block resolves this issue.

cc @mtias re starter post content being too long and consuming too much memory. It sounds like we need to do some memory profiling, and testing on lower-perfomance devices, because there are likely already some issues here even without Redux devtools.

Using real-world examples that are much longer and more complicated than the demo content, we should set up some stress/performance tests with very long content as well. Probably OK for something like our full-content tests, but I'm not sure if we want to run them on every build.

cc @iseulde, I wonder if something like #847 would also help with this.

@mtias
Copy link
Member

mtias commented May 29, 2017

@nylen definitely, it's time to start looking at how to profile and test different post sizes. Do you want to look into how we could set this up? I have a few real-world examples from Matt of posts that are quite long and complicated, and which struggle in tinymce already.

I'm also a bit concerned with validating individual block shapes through schemas (#914) because of how expensive things could get.

@ellatrix
Copy link
Member

I wonder if something like #847 would also help with this.

@nylen I'm unsure about the impact. Something I have in mind that we could try is just adding the contentEditable to the Editable, but either only initialise it once it receives focus, or initialise one instance with the same settings and attach that to focussed Editable. A condition for this is that we don't rely on TinyMCE's parsing and validation and that we can just set the "raw" content. Removing this parsing alone should also improve initialisation speed.

I have a few real-world examples from Matt of posts that are quite long and complicated, and which struggle in tinymce already.

@mtias Could you share those?

@nylen
Copy link
Member Author

nylen commented May 29, 2017

Do you want to look into how we could set this up? I have a few real-world examples from Matt of posts that are quite long and complicated, and which struggle in tinymce already.

Sure, can you share those examples with us somehow? We'll probably want to adapt them to contain blocks. I'm also aware of a couple others inside a8c that would need sensitive information removed.

I think something like test/fixtures but with memory/speed profiling (and without running on every build) would work well.

@nylen
Copy link
Member Author

nylen commented May 29, 2017

I'm also a bit concerned with validating individual block shapes through schemas (#914) because of how expensive things could get.

This seems true of any way of doing the things listed in #914 (mainly future enhancements). I think it is better for us to handle this complexity in a reusable way than to push it onto plugin authors.

@aduth
Copy link
Member

aduth commented Jun 7, 2017

On a hunch I dropped the list blocks from post-content.js and the DevTools load without issue. It's not a general issue of content size, but rather how we're setting into attributes (tracked by Redux state) the list block's TinyMCE instance.

It opens a question about the sorts of values we want to allow to be set into attributes. Should they be non-complex values only (numbers, strings, booleans)? How do we enforce this, or do we even want to? What alternatives do we propose instead (in this case, perhaps in block component state).

Still not totally clear on why the TinyMCE instance specifically causes the page to crash.

cc @EphoxJames

@youknowriad
Copy link
Contributor

Oh! I missed this, yeah we should assume that all the attributes we pass to setAttributes are attributes to be serialized (in the content or the comment), if we need complex attributes like this, we should use Component's local state. (Moving the controls to the edit function helps here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants