-
Notifications
You must be signed in to change notification settings - Fork 661
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
Helmet 5.0.0: The no-API release #246
Conversation
…ating a more declarative API. Fully backwards compatible.
…t-helmet into feature/declarative-api
…t-helmet into feature/declarative-api
…t-helmet into feature/declarative-api
… into feature/declarative-api
…t-helmet into feature/declarative-api
… into feature/declarative-api
…t-helmet into feature/declarative-api
… into feature/declarative-api
…t-helmet into feature/declarative-api
…' of github.com:nfl/react-helmet into feature/declarative-api
…t-helmet into feature/declarative-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done. Thanks for formalizing our hack session. Looks great!
Just a couple typos and you should be good to go.
src/Helmet.js
Outdated
* @param {Array} noscript: [{"innerHTML": "<img src='http://mysite.com/js/test.js'"}] | ||
* @param {Array} style: [{"type": "text/css", "cssText": "div{ display: block; color: blue; }"}] | ||
* @param {Array} meta: [{"name": "description", "content": "Test description"}] | ||
* @param {Array} noscript: [{TAG_PROPERTIES.INNER_HTML: "<img src='http://mysite.com/js/test.js'"}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"innerHTML"
src/Helmet.js
Outdated
* @param {Function} onChangeClientState: "(newState) => console.log(newState)" | ||
* @param {Array} script: [{"type": "text/javascript", "src": "http://mysite.com/js/test.js"}] | ||
* @param {Array} style: [{"type": "text/css", TAG_PROPERTIES.CSS_TEXT: "div{ display: block; color: blue; }"}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"cssText"
HelmetExport.renderStatic = HelmetExport.rewind; | ||
|
||
export {HelmetExport as Helmet}; | ||
export default HelmetExport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well played
This seems to have been committed to master causing some confusion with the readme (instead of to a branch for the new release) One such confusion: #249 |
The React community is too quick, I literally landed this a few hours ago! We'll be publishing the new API shortly. |
@doctyper consider a I spent an hour this morning trying to figure out why my code wasnt working only to realize that the versions did not match up :-( |
I do apologize, but do note that master branches do not always align with currently published versions. This goes for any repository. I would instead direct you to the npm-hosted README, which does currently point to the published release. https://www.npmjs.com/package/react-helmet As a general policy we do not use release branches @nfl. |
I understand internally @nfl may not use release branches, but as a consumer of your open source code my team cannot use a tool that introduces breaking changes into master with no warning |
@doctyper We do like the project though and we use it extensively -- not saying we are jumping ship :-) Just raising concerns is all as breaking changes on master can get the community a little confused without a release. |
@nlubin-lv Thanks for keeping us honest, we should have held off on the README changes until we bumped the version. |
Hmm, this is rather confusing because I thought I could then put some React components in there, but I'm getting Helmet expects a string as a child of <title>. Did you forget to wrap your children in braces? So using react-intl FormattedMessage doesn't work which would be what I thought the point of this would be |
@ntucker it seems you can only put the components that react-helmet accepts as children to I believe that was on purpose as not any element can go in the head tag. I needs a string as a child of title due to the fact that the title gets manipulated by the module to add on attributes and other data. |
In case it is helpful to anyone in the future, I wrote a codemod to transform all of our |
Helmet 5
@cwelch5 and I got inspired during React Conf 2017 and worked together to overhaul the Helmet API. This release will replace the current API with, well, no API (...sort of).
Helmet 5 will allow you to rewrite this:
to this:
That's it. There's no need to pass in props or large objects / arrays. Helmet now takes plain HTML tags and outputs plain HTML tags. It's dead simple, and React beginner friendly.
Minimal API
The Helmet API has been reduced to just two optional convenience props and an optional callback:
Reference Example
For a more concrete example, here is the current README example converted to Helmet 5:
Todo
We have a few more items to check off, but we are excited for this release!
Helmet.renderStatic()
alias toHelmet.rewind()
to reduce confusion on what it does{Helmet}
export for full ES Module compatibility<body>
attributesrequestIdleCallback
for DOM writesPreview
Please feel free to help us out by installing and test-driving our preview release. Feedback is much appreciated!
or: