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

fix: WrappedValue.unwrap empty string behavior #6900

Merged
merged 7 commits into from
Mar 14, 2019

Conversation

speigg
Copy link
Contributor

@speigg speigg commented Feb 13, 2019

Currently:

var value = new WrappedValue('') // passing empty string
console.log(WrappedValue.unwrap(value) === '' ) // prints "false"

Now:

var value = new WrappedValue('') // passing empty string
console.log(WrappedValue.unwrap(value) === '' ) // prints "true"

PR Checklist

What is the current behavior?

What is the new behavior?

Fixes/Implements/Closes #[Issue Number].

Currently:
```
var value = new WrappedValue('') // passing empty string
console.log(WrappedValue.unwrap(value) === '' ) // prints "false"
```

Now:
```
var value = new WrappedValue('') // passing empty string
console.log(WrappedValue.unwrap(value) === '' ) // prints "true"
```
@ghost ghost added the ♥ community PR label Feb 13, 2019
@ghost ghost assigned manoldonev Feb 22, 2019
@ghost ghost added in progress and removed ♥ community PR labels Feb 22, 2019
@manoldonev manoldonev changed the title Fix WrappedValue.unwrap fix: WrappedValue.unwrap empty string behavior Feb 22, 2019
@manoldonev
Copy link
Contributor

@speigg there are failing unit tests with this change:

JS: Test: --- [XML-DECLARATION.test_TabViewHasCorrectParentChain] FAILED: Cannot use 'in' operator to search for 'wrapped' in true, Stack: TypeError: Cannot use 'in' operator to search for 'wrapped' in true

You can run the unit tests locally like this:

cd tests
tns run android

Copy link
Contributor

@vakrilov vakrilov left a comment

Choose a reason for hiding this comment

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

As @manoldonev pointed out - the proposed solution fails when unwrap is given a boolean value.
Lets just make an instanceof check:

return (value instanceof WrappedValue) ? value.wrapped : value;

Also, can you add some unit-tests about this case. You can check this guide on how to do that.

@vakrilov vakrilov assigned vakrilov and unassigned manoldonev Feb 26, 2019
@vakrilov
Copy link
Contributor

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Feb 27, 2019

The cla-bot has been summoned, and re-checked this pull request!

@ghost ghost assigned manoldonev Mar 12, 2019
@speigg
Copy link
Contributor Author

speigg commented Mar 12, 2019

@manoldonev thanks for taking care of this, I haven't had time to work on this myself

@manoldonev
Copy link
Contributor

test

@manoldonev manoldonev merged commit 0482460 into NativeScript:master Mar 14, 2019
@ghost ghost removed the in progress label Mar 14, 2019
@lock
Copy link

lock bot commented Mar 17, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants