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.

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

stop adding null string key when there is a ref #1254

Merged
merged 1 commit into from
Nov 4, 2017

Conversation

bdwain
Copy link
Contributor

@bdwain bdwain commented Oct 12, 2017

This fixes #1240 . Since react turns all valid (aka not undefined) keys into strings in react.createElement, it made sense to just remove falsey keys from nodes, so that when they were passed back through createElement, they didn't get turned into strings.

The tests I added fail with errors like

AssertionError: expected 'null' to equal null

without the changes I made.

@bdwain
Copy link
Contributor Author

bdwain commented Oct 16, 2017

is there anything else needed before this is looked at? It'd be great to get it merged. Idk how many people it's affecting but I know my project can't upgrade to react 16 until it's done.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This LGTM - it needs a rebase on the CLI to remove the merge commit, and it'll need a few more reviews.

@bdwain
Copy link
Contributor Author

bdwain commented Oct 16, 2017

cool thanks. i rebased off of master

Copy link
Collaborator

@nfcampos nfcampos left a comment

Choose a reason for hiding this comment

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

LGTM

@bdwain bdwain force-pushed the fix-null-key branch 2 times, most recently from 3a5e564 to bdccbd8 Compare October 19, 2017 04:55
@bdwain
Copy link
Contributor Author

bdwain commented Oct 19, 2017

I rebased this. Does it need more approvals?

@ljharb
Copy link
Member

ljharb commented Oct 19, 2017

Yes, I'd like a couple more reviewers to take a look first.

@bdwain
Copy link
Contributor Author

bdwain commented Oct 30, 2017

Hi @ljharb is there anything i can do to get this reviewed and merged? I know it's blocking me from updating to react 16, and I'm sure it's blocking others as well.

@ljharb
Copy link
Member

ljharb commented Oct 30, 2017

I'll give it a try locally.

@bdwain
Copy link
Contributor Author

bdwain commented Nov 3, 2017

any luck with this? It's getting hard to keep it up to date because merge conflicts keep happening as master updates.

@ljharb
Copy link
Member

ljharb commented Nov 4, 2017

Still working on it :-) I'll keep it up to date for you tho!

@ljharb ljharb merged commit 8c21e40 into enzymejs:master Nov 4, 2017
@bdwain
Copy link
Contributor Author

bdwain commented Nov 4, 2017

thanks!

@bdwain bdwain deleted the fix-null-key branch November 4, 2017 18:00
ljharb added a commit to ljharb/enzyme that referenced this pull request Nov 5, 2017
 - [fix] stop adding null string key when there is a ref (enzymejs#1254)
@tryggvigy
Copy link

I believe #1511 was introduced in this PR

jquense pushed a commit to jquense/enzyme that referenced this pull request Feb 23, 2018
 - [fix] stop adding null string key when there is a ref (enzymejs#1254)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Components with refs getting "null" (string) key even if no key is set
4 participants