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.

Sign up for GitHub

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

Jump to bottom

Add possibility to make Helmet Component Class on demand #296

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add possibility to make Helmet Component Class on demand #296

wants to merge 3 commits into from

Conversation

pristas-peter
Copy link

@pristas-peter pristas-peter commented Jun 21, 2017 •
edited
Loading

We are streaming server response (not using ReactDomServer.renderToString) and we need singleton instance of Helmet Component Class per each request since it is using static variable on wrapped component (streaming is async, so there could be race conditions)...

@CLAassistant
Copy link

CLAassistant commented Jun 21, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #296 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage   98.94%   98.95%   +<.01%     
==========================================
  Files           3        3              
  Lines         285      287       +2     
==========================================
+ Hits          282      284       +2     
  Misses          3        3
Impacted Files Coverage Δ
src/Helmet.js 100% <100%> (ø) ⬆️

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...026c5b5. Read the comment docs.

@oyeanuj
Copy link

oyeanuj commented Sep 23, 2017

@pristas-peter Do you think this will work with React 16, renderToNodeStream method as well?

@pristas-peter
Copy link
Author

yes, already using it.

@gabemeola gabemeola mentioned this pull request Oct 3, 2017
Open
@goatslacker
Copy link

@pristas-peter how are you passing the Helmet instance around to children, through context?

@praneshr
Copy link

praneshr commented Oct 7, 2017

@pristas-peter Can you add an example to show how it works with React 16, renderToNodeStream?

@patrickgordon
Copy link

I would love to see an example of how you're using this @pristas-peter

@pristas-peter
Copy link
Author

pristas-peter commented Oct 19, 2017 •
edited
Loading

Sorry to reply so late...
Basically what we do is:

  • we have custom router solution based on router5 (not react-router), which preloads all data which is needed to do a render on server
  • the result of this loading process also contains jsx with helmet elements for , the JSX is basically <title>{data.title}</title> ... , where Helmet class is not the global imported class from helmet module, but the singleton class created with patch from this Pull Request
  • after that, before sending the streamed response with React, we manually invoke React.renderToStaticMarkup on the returned helmet element, so that Helmet class has all properties available through Helmet.renderStatic(), which we can use in stream...

Hope it is somehow clear, feel free to ask any questions

Edit: the code is also universal, on client we just pass the "Helmet element" to render function of the root element

@goatslacker
Copy link

before sending the streamed response with React, we manually invoke React.renderToStaticMarkup on the returned helmet element

So you're rendering twice on the server? Once via renderToStaticMarkup for side effects and once so you can stream the response down to the client?

I'm curious, why stream at all if you already have the HTML and can send that down to your users?

@pristas-peter
Copy link
Author

pristas-peter commented Oct 20, 2017 •
edited
Loading

No, not twice... RenderToStaticMarkup is only run on a small helmet element<Helmet><title>....</Helmet>(yes that is synchronous, but it does not matter that much because it is a very small markup).

@DNFcode
Copy link

DNFcode commented Oct 25, 2017 •
edited
Loading

@pristas-peter Thanks for the idea of solving this issue!
But I don't really understand why do you need a separate instance of Helmet.
Do you initialize renderToNodeStream of React app before you are rendering contents of <head>?

It just works for me without separate instance, and I don't see any reasons for race conditions. Maybe I'm missing something?

@pristas-peter
Copy link
Author

try to simulate very high number of simultaneous requests, I know that that the probability isn't that hight that the context switch of node process switches between inside rendering two helmet elements, but chance is still there, but very minimal, but it can happen...

@antonybudianto antonybudianto mentioned this pull request Oct 30, 2017
Closed
@vsc-github
Copy link

Is it possible to use react-helmet with react-router while streaming html via renderToNodeStream ?

@adam-26
Copy link

adam-26 commented Dec 14, 2017

@vsc-github, see this comment in issue 322 for an alternative implementation with react-router. Internally it uses a modified version of react-helmet. The API is extremely different to react-helmet so it may not suit your needs, but it does support SSR with streams.

@pristas-peter
Copy link
Author

This PR was created on 21 Jun 2017. Can somebody comment if this can be merged?

@lopezjurip
Copy link

Any update on this? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@pristas-peter @CLAassistant @oyeanuj @goatslacker @praneshr @patrickgordon @DNFcode @vsc-github @adam-26 @lopezjurip