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

Allow empty strings as key props #1524

Merged
merged 1 commit into from
Feb 23, 2018
Merged

Allow empty strings as key props #1524

merged 1 commit into from
Feb 23, 2018

Conversation

tryggvigy
Copy link

This PR will

Why?

React allows empty string key props. We should too:

const wrapper = shallow(
  <div>
    {[''].map(s => <span key={s}>{s}</span>)}
  </div>
);
wrapper.at(0).key() // === `undefined` on master,  === `''` on this PR

@tryggvigy tryggvigy mentioned this pull request Feb 11, 2018
Closed
10 tasks
tryggvigy
tryggvigy commented Feb 11, 2018
View reviewed changes
packages/enzyme-adapter-utils/src/Utils.js
@@ -153,6 +153,10 @@ export function flatten(arrs) {
return flatArrs;
}

export function ensureKeyOrUndefined(key) {
return key || (key === '' ? '' : undefined);
Copy link
Author

@tryggvigy tryggvigy Feb 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check could be more restrictive.

 export function ensureStringOrUndefined(key) {
  return typeof key === 'string' ? key : undefined;
}

Are there situations where a key can be anything other than a string or a falsey value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react stringifies all valid keys before storing them, and a key is valid if it is not undefined. So i think this workes either way. I think it's probably best to leave it as you have now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe typeof key === 'string' ? key : undefined?

ljharb
ljharb approved these changes Feb 23, 2018
View reviewed changes
packages/enzyme-adapter-utils/src/Utils.js
@@ -153,6 +153,10 @@ export function flatten(arrs) {
return flatArrs;
}

export function ensureKeyOrUndefined(key) {
return key || (key === '' ? '' : undefined);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe typeof key === 'string' ? key : undefined?

@ljharb ljharb force-pushed the allow-empty-string-key-props branch from 8ffd0f1 to f080dc9 Compare February 23, 2018 07:48
@ljharb ljharb merged commit f080dc9 into enzymejs:master Feb 23, 2018
@tryggvigy tryggvigy deleted the allow-empty-string-key-props branch February 23, 2018 23:13
ljharb added a commit that referenced this pull request Jul 9, 2018
@ljharb
[enzyme-adapter-utils] v1.4.0
6d0d2c4 
 - [New] Add event name mapping for animation events
 - [Fix] Allow empty strings as key props (#1524)
 - [Deps] remove unneeded `lodash` dep, update `object.assign`
 - [Dev Deps] update `babel-cli`, `babel-core`, `babel-plugin-transform-replace-object-assign`, `babel-preset-airbnb`, `chai`, `coveralls`, `eslint`, `eslint-config-airbnb`, `eslint-plugin-import`, `eslint-plugin-jsx-a11y`, `eslint-plugin-react`, `json-loader`, `lerna`, `karma-firefox-launcher`, `prop-types`, `lerna`, `rimraf`, `webpack`
ljharb added a commit that referenced this pull request Jul 9, 2018
@ljharb
[enzyme-adapter-react-14] v1.0.6
a37387a 
 - [fix] call ref for a root element (#1541)
 - [fix] Allow empty strings as key props (#1524)
 - [fix] Allow empty strings as key props
 - [fix] correct the adapter class name
 - [fix] `mount`: make sure it works with native arrow functions (#1667)
 - [refactor]: add “lifecycles” adapter option (#1696)
 - [refactor] use `react-is` package
 - [deps] remove unneeded `lodash` dep; update `prop-types`, `enzyme-adapter-utils`, `object.assign`
 - [dev deps] update `babel-cli`, `babel-core`, `babel-plugin-transform-replace-object-assign`, `babel-preset-airbnb`, `chai`, `coveralls`, `eslint`, `eslint-config-airbnb`, `eslint-plugin-import`, `eslint-plugin-jsx-a11y`, `eslint-plugin-react`, `lerna`, `json-loader`, `karma-firefox-launcher`, `prop-types`, `lerna`, `rimraf`, `webpack`
ljharb added a commit that referenced this pull request Jul 9, 2018
@ljharb
[enzyme-adapter-react-15.4] v1.0.6
1dd038e 
 - [fix] actually invoke the setState callback (#1465, #1453)
 - [fix] add missing support for animation events (#1569)
 - [fix] call ref for a root element (#1541)
 - [fix] Allow empty strings as key props (#1524)
 - [fix] correct the adapter class name
 - [fix] `mount`: make sure it works with native arrow functions (#1667)
 - [refactor]: add “lifecycles” adapter option (#1696)
 - [refactor] use `react-is` package
 - [deps] remove unneeded `lodash` dep; update `prop-types`, `enzyme-adapter-utils`, `object.assign`
 - [dev deps] update `babel-cli`, `babel-core`, `babel-plugin-transform-replace-object-assign`, `babel-preset-airbnb`, `chai`, `coveralls`, `eslint`, `eslint-config-airbnb`, `eslint-plugin-import`, `eslint-plugin-jsx-a11y`, `eslint-plugin-react`, `lerna`, `json-loader`, `karma-firefox-launcher`, `prop-types`, `lerna`, `rimraf`, `webpack`
ljharb added a commit that referenced this pull request Jul 9, 2018
@ljharb
[enzyme-adapter-react-15] v1.0.6
1187a6b 
 - [fix] actually invoke the setState callback (#1465, #1453)
 - [fix] add missing support for animation events (#1569)
 - [fix] call ref for a root element (#1541)
 - [fix] Allow empty strings as key props (#1524)
 - [fix] correct the adapter class name
 - [fix] `mount`: make sure it works with native arrow functions (#1667)
 - [refactor]: add “lifecycles” adapter option (#1696)
 - [refactor] use `react-is` package
 - [deps] remove unneeded `lodash` dep; update `prop-types`, `enzyme-adapter-utils`, `object.assign`
 - [dev deps] update `babel-cli`, `babel-core`, `babel-plugin-transform-replace-object-assign`, `babel-preset-airbnb`, `chai`, `coveralls`, `eslint`, `eslint-config-airbnb`, `eslint-plugin-import`, `eslint-plugin-jsx-a11y`, `eslint-plugin-react`, `lerna`, `json-loader`, `karma-firefox-launcher`, `prop-types`, `lerna`, `rimraf`, `webpack`
ljharb added a commit that referenced this pull request Aug 8, 2018
@ljharb
[enzyme-adapter-react-16] v1.2.0
f5a648c 
 - [new] add `isFragment` (#1733)
 - [new] Add `displayNameOfNode`, `isValidElementType`
 - [new] `mount`: add `hydrateIn` option (#1317, #1707)
 - [new] Add support for react context element types (#1513)
 - [new] `shallow`: Add getSnapshotBeforeUpdate support (#1657)
 - [fix] portals and roots may render fragments (#1733)
 - [fix] add missing support for animation events (#1569)
 - [fix] `shallow`: SFCs do not get a `this` (#1703)
 - [refactor]: add “lifecycles” adapter option (#1696)
 - [fix] call ref for a root element (#1541)
 - [fix] Allow empty strings as key props (#1524)
 - [fix] Fix ShallowWrapper for array-rendering components (#1498)
 - [refactor] use `react-is` package
 - [meta] ensure a license and readme is present in all packages when published
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdwain bdwain bdwain approved these changes

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
package: enzyme-adapter-utils package: react adapter: 0.13 package: react adapter: 0.14 package: react adapter: 15 package: react adapter: 15.4 package: react adapter: 16 semver: patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tryggvigy @ljharb @bdwain