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

Preserve null in babel-types' clone and deepClone. #4853

Merged
merged 2 commits into from Nov 17, 2016

Conversation

NTillmann
Copy link
Contributor

@NTillmann NTillmann commented Nov 17, 2016

Q A
Bug fix? Yes
Breaking change? No
New feature? No
Deprecations? No
Spec compliancy? No
Tests added/pass? Yes
Fixed tickets Fixes #4852
License MIT
Doc PR
Dependency Changes

Preserve null in babel-types' clone and deepClone.
(The current behavior turns null into {}, which is not equivalent. Null is a valid expression used to represent missing array elements. The Printer in particular cannot generate code for {}, but it does handle null.)
Add tests.

@codecov-io
Copy link

codecov-io commented Nov 17, 2016

Current coverage is 89.36% (diff: 100%)

No coverage report found for master at b86f618.

Powered by Codecov. Last update b86f618...2d07528

@hzoo
Copy link
Member

hzoo commented Nov 17, 2016

Cool - what about a test case for a node that contains null for deepClone and I assume undefined is good?

@hzoo hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Nov 17, 2016
@hzoo
Copy link
Member

hzoo commented Nov 17, 2016

I would add your original test case in the tests? [,0] (I removed the quotes in the description)

Adding tests.
Updating last test to reflect original test case [,0]
@NTillmann
Copy link
Contributor Author

Thanks for the feedback! I updated the implementation to also handle undefined, and added tests for top-level undefined and nested explicit null. However, I don't think there is any valid nested place for undefined --- if you know one, I could add a test for it. Otherwise, as far as I know, null is really the only special value where things went wrong before.

Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

lgtm

@hzoo hzoo merged commit efefe82 into babel:master Nov 17, 2016
@hzoo
Copy link
Member

hzoo commented Nov 17, 2016

Awesome! Thanks for contributing 👌

panagosg7 pushed a commit to panagosg7/babel that referenced this pull request Jan 17, 2017
* Preserve null in babel-types' clone and deepClone.
* Also respect undefined when cloning.
* Updating last test to reflect original test case "[,0]"
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloneDeep in babel-types doesn't preserve null
4 participants