fix(children): handle empty array children #1789
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1789 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 142 142
Lines 2441 2441
=======================================
Hits 2435 2435
Misses 6 6
Continue to review full report at Codecov.
|
src/lib/childrenUtils.js
Outdated
| if (children === null) return true | ||
| if (Array.isArray(children) && children.length === 0) return true | ||
| return false | ||
| } |
There was a problem hiding this comment.
Please take attention, that isNil checks if value is null or undefined.
There was a problem hiding this comment.
Understood. Sorry, I undid the last commit. I hastily made changes to satisfy the linter.
src/lib/childrenUtils.js
Outdated
| */ | ||
| export const isNil = (children) => { | ||
| if (children == null) return true // eslint-disable-line | ||
| if (Array.isArray(children) && children.length == 0) return true // eslint-disable-line |
There was a problem hiding this comment.
I'd prefer we do not ignore the linter. Please use ===.
There was a problem hiding this comment.
The Lodash implementation uses ==, the deeper === actually breaks things. Suggestion?
There was a problem hiding this comment.
Sure, the == simply coerces null/undefined comparisons to return true. We can just explicitly list those. [].length always returns a number so a triple equal there should be safe. Something like:
export const isNil = children => {
return children === null
|| children === undefined
|| Array.isArray(children) && children.length === 0
}
src/lib/childrenUtils.js
Outdated
| @@ -16,3 +16,14 @@ export const someByType = (children, type) => _.some(Children.toArray(children), | |||
| * @returns {undefined|Object} | |||
| */ | |||
| export const findByType = (children, type) => _.find(Children.toArray(children), { type }) | |||
|
|
|||
| /** | |||
| * Tests if any children. | |||
There was a problem hiding this comment.
How about something like: Tests if children are nil in React and Preact.?
src/lib/childrenUtils.js
Outdated
| * @param {Object} children The children prop of a component. | ||
| * @returns {Boolean} | ||
| */ | ||
| export const isNil = (children) => { |
There was a problem hiding this comment.
Since this is now a very critical util for the lib, let's get some tests on it. You can see other lib tests in test/specs/lib.
|
Looks great, thanks for the rapid updates! We'll get this merged when CI passes. |
|
@levithomason Thank you for reviewing so quickly! |
|
No problem, if CI lint issues are getting in your way note that you can check them locally with: npm run lint:fixThis will fix many issues and report the remaining errors. You can safely ignore the |
|
Yeah, I tried that before. It complains about the configuration being invalid. Which version of eslint are you running? |
|
Hm, as long as you have node >=6 you should be able to do: You can see all the versions used in |
|
The global installation of eslint is being called, I had 4.0. Reverting to 3.x resolved. Should probably be pegged explicitly in the project. |
|
Oh yucky, we're apparently using That said, we're good here now. Thanks much! |
|
As a heads up, |
|
Awesome news! Sorry about the linting debacle 😅 |
|
No worries, totally on me there. |
|
Released in |
This PR properly tests if children are nil. Fixes compatibility issue with Preact.