-
Notifications
You must be signed in to change notification settings - Fork 660
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: requestIdleCallback support #248
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
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.
Couple questions...
@@ -28,58 +28,59 @@ | |||
"react": ">=15.0.0" | |||
}, | |||
"dependencies": { | |||
"deep-equal": "1.0.1", | |||
"object-assign": "^4.0.1", | |||
"deep-equal": "^1.0.1", |
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.
Why are we loosening the rules? we've been burned in the past. Shouldn't we keep them locked?
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.
ah, disregard, missed the yarn.lock
src/Helmet.js
Outdated
@@ -552,8 +180,18 @@ const Helmet = (Component) => class HelmetWrapper extends React.Component { | |||
nestedChildren && | |||
typeof nestedChildren !== "string" | |||
) { | |||
console.warn(`Helmet expects a single string as a child of ${child.type}`); | |||
if (!VALID_TAG_NAMES.some(name => child.type === name)) { |
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.
This is great checking, but only does VALID_TAG_NAMES check if the tag has children, can we maybe move that check higher to check all children while we are at it.
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.
You're right. I updated the logic and added a test for self-closing invalid elements.
@@ -17,6 +17,10 @@ export const TAG_NAMES = { | |||
TITLE: "title" | |||
}; | |||
|
|||
export const VALID_TAG_NAMES = Object |
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.
pretty cool
const generateTitleAsString = (type, title, attributes, encode) => { | ||
const attributeString = generateElementAttributesAsString(attributes); | ||
return attributeString | ||
? `<${type} ${HELMET_ATTRIBUTE}="true" ${attributeString}>${encodeSpecialCharacters(title, encode)}</${type}>` |
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.
encodeSpecialCharacters
- is this the "functional" way of doing it? seems confusing, what if the check for encode
was here so people don't think it's always encoded:
encode ? encodeSpecialCharacters(title) : title
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.
if encode is false, encodeSpecialCharacters
won't encode the supplied string, but it will ensure the returned value is a string.
This PR adds support for
requestIdleCallback
, which should greatly improve the performance of Helmet.