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

Adds support for synchronously updated tags (Closes #291) #297

Merged
merged 1 commit into from Jul 26, 2017

Conversation

@jaysoo
Copy link
Contributor

@jaysoo jaysoo commented Jun 22, 2017

This PR is to address the issue with FOUC as outlined in #291.

The code changes involve splitting the updates into two groups: deferred (default), and sync. The sync behaviour is opt-in using the new defer={false} prop on Helmet.

<div>
  <Helmet defer={false}>
    {/* These updates will be sync during the render cycle.
      * By including <style> tags here, you can avoid FOUC
      * of the rendering element as outlined in #291. 
      */}
  </Helmet>
  <Helmet>
    {/* These updates will be async during the render cycle. */}
  </Helmet>
</div>

Two new tests have been added to both declarative and original API. They both test that updates can happen before requestIdleCallback and during.

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Jun 22, 2017

CLA assistant check
All committers have signed the CLA.

@jaysoo jaysoo mentioned this pull request Jun 22, 2017
@codecov
Copy link

@codecov codecov bot commented Jun 22, 2017

Codecov Report

Merging #297 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   98.94%   98.96%   +0.02%     
==========================================
  Files           3        3              
  Lines         285      291       +6     
==========================================
+ Hits          282      288       +6     
  Misses          3        3
Impacted Files Coverage Δ
src/HelmetConstants.js 100% <ø> (ø) ⬆️
src/Helmet.js 100% <ø> (ø) ⬆️
src/HelmetUtils.js 98.65% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c947ede...b22e2f4. Read the comment docs.

@jaysoo jaysoo force-pushed the jaysoo:issues/291-fouc branch 2 times, most recently from d0943b0 to 0f1cbdb Jun 22, 2017
it("executes synchronously when defer={true} and async otherwise", done => {
ReactDOM.render(
<Helmet>
<script defer={false}>

This comment has been minimized.

@doctyper

doctyper Jun 22, 2017
Collaborator

defer is (I believe) specific to the <script> tag. So this solution wouldn't work for style or any other tag. Perhaps it makes more sense to bubble up the attribute to the <Helmet> wrapper? That would give devs the flexibility to sync-render the entire Helmet instance, rather than the individual tag.

<Helmet defer={false}>

This comment has been minimized.

@doctyper

doctyper Jun 22, 2017
Collaborator

Option 2 would be to keep this syntax, but strip out invalid defer attributes before we write to the DOM.

This comment has been minimized.

@jaysoo

jaysoo Jun 22, 2017
Author Contributor

Good point. Maybe it should just go to <Helmet>, then the user can decide whether to separate tags into the defer <Helmet> or not. Would make the API easier to support as well.

I'll redo the PR today.

… sync or deferred. Default is defer={true}. Closes #291
@jaysoo jaysoo force-pushed the jaysoo:issues/291-fouc branch from 0f1cbdb to b22e2f4 Jun 23, 2017
@jaysoo
Copy link
Contributor Author

@jaysoo jaysoo commented Jun 23, 2017

@doctyper Update the PR so that defer is on Helmet, not individual tags. Tags can be grouped using:

<div>
  <Helmet defer={false}>
    {/* sync tags... */}
  </Helmet>
  <Helmet>
    {/* async tags... */}
  </Helmet>
</div>
Copy link
Member

@cwelch5 cwelch5 left a comment

Great PR, thanks!

@arestov
Copy link

@arestov arestov commented Jul 8, 2017

Hey! Do you have any plans to merge it?

@arestov
Copy link

@arestov arestov commented Jul 26, 2017

@doctyper @cwelch5 Do you have any plans to merge it?

@cwelch5 cwelch5 merged commit 6a3d3bf into nfl:master Jul 26, 2017
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 98.94%)
Details
codecov/project 98.96% (+0.02%) compared to c947ede
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@arestov
Copy link

@arestov arestov commented Jul 27, 2017

@cwelch5 could you please publish new npm version with merged changes? 🙏

@Shangyunliang
Copy link

@Shangyunliang Shangyunliang commented Oct 20, 2018

Great! but when i use link tag. it dose work. css will be load at first. but not work so well.
do you have any suggestions?
image

@jaysoo jaysoo deleted the jaysoo:issues/291-fouc branch Apr 9, 2019
delete window.__spy__;
});

it("executes synchronously when defer={true} and async otherwise", done => {

This comment has been minimized.

@skvale

skvale Apr 1, 2020

shouldn't this be executes synchronously when defer={false} instead?

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

Successfully merging this pull request may close these issues.

None yet

7 participants