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

refactor: add timeout to avoid FOUC #221

Closed
wants to merge 2 commits into from

Conversation

viankakrisna
Copy link

What kind of change does this PR introduce?
Enhancement

Did you add tests for your changes?
no, but if this approach is acceptable, will do

If relevant, did you update the README?

Summary
Add hacks for hiding document while style-loader is appending styles to the head (prevents FOUC) will fix issue found on facebook/create-react-app#591

Does this PR introduce a breaking change?
I don't think so

Other information
because this is a hack, probably better to add config to this behavior.

@jsf-clabot
Copy link

jsf-clabot commented Apr 24, 2017

CLA assistant check
All committers have signed the CLA.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 24, 2017

@viankakrisna I'm in general not sure if this would be a good idea tbh, since FOUC in a development environment isn't really a problem, but in case it works without any possible side effects we can add a solution 😛 What about

document.readyState
document.onreadystatechange

?

@viankakrisna
Copy link
Author

Yeah, i also not sure to be honest, but it seems that it blocks CRA to use source map in dev per linked pr. I don't think onreadystatechange is what we looking for, cause it's only check if the dom ready, not script done executing. Is it not?

To be safe, I think this should be hidden behind a config flag, something like hideDomOnRender

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 25, 2017

To be safe, I think this should be hidden behind a config flag, something like hideDomOnRender

Nope sry an additiontal option adds for this would add too much noise to the loader for less reason and is pit for confusion.

👍 if it works 'out-of-the-box' without regressions
👎 for an additional option fo reason stated above ☝️

cc @bebraw @d3viant0ne @evilebottnawi @ekulabuhov

@michael-ciniawsky michael-ciniawsky self-assigned this Apr 25, 2017
@michael-ciniawsky michael-ciniawsky changed the title Add hacks for hiding FOUC refactor: add timeout to avoid FOUC Apr 25, 2017
@michael-ciniawsky michael-ciniawsky added this to Feature in Dashboard Apr 25, 2017
@michael-ciniawsky michael-ciniawsky moved this from Feature to Refactor in Dashboard Apr 25, 2017
@alexander-akait
Copy link
Member

alexander-akait commented Apr 25, 2017

@viankakrisna I would like to see an example where fouc is a real problem in the development (there can be a lot of css or something else to evaluate how much this adds to the inconvenience), maybe example repo can help with this

@michael-ciniawsky
Copy link
Member

@evilebottnawi How problematic would you say is adding a timeout, or wait for readyState ? Could this cause other regressions ? There is also the possiblity to track scripts

@alexander-akait
Copy link
Member

@michael-ciniawsky hm, it need investigation

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 25, 2017

@evilebottnawi Don't bother too much 😛 I don't think we'll add this, since it could introduce side effects for less obvious benefit

@alexander-akait
Copy link
Member

@michael-ciniawsky today i don't have many times, but tomorrow - yes 😄 just don't close and i see on this later 😄

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 25, 2017

Yep 👍 you rush to close it, I have no objections against it in case it 'simply works' 😛

@viankakrisna
Copy link
Author

@evilebottnawi i've tried to create a convoluted example with 10.000 lines of css and 10.000 lines of jsx (even chrome takes awhile to parse the sourcemap), and the FOUC is happening for about 1-2s.

The real benefit i see for this is for the people that turned off of using it for styling because of FOUC (even if it happens initially for a split second). And of course, to get the sourceMap option turned for styling on CRA 😄

here's the link for the example https://github.com/viankakrisna/create-react-app/tree/foucdemo

another idea: maybe instead of blanking out we should print out a popup on the corner of the screen like browsersync does, that we are in the middle of injecting css?
https://youtu.be/heNWfzc7ufQ?t=4m24s
second thought: we are blanking the document so i don't think we can print anything else. except if the opacity is 0.5 with a text on the corner of the screen.

I don't know, just throwing up some ideas here...

@alexander-akait
Copy link
Member

@viankakrisna thanks for long answer 😄 tomorrow i try to find time on this

@michael-ciniawsky
Copy link
Member

@viankakrisna @evilebottnawi Did you test this further in terms of possible regressions ?

@alexander-akait
Copy link
Member

@michael-ciniawsky no time do this immediately, tomorrow there is time just to see everything, sorry 😭

@alexander-akait
Copy link
Member

@viankakrisna very strange i have fouc with this solution only less often

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented May 2, 2017

https://developer.mozilla.org/en-US/docs/Web/API/Document/readyState
https://developer.mozilla.org/en-US/docs/Web/API/Document/onafterscriptexecute (Non-standard)

Something like that maybe, but this neither isn't really urgent nor is it a clean solution, so closing this for now 😛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants