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

[BUGFIX release] Fix get with empty string #11205

Merged
merged 1 commit into from
May 18, 2015

Conversation

paddyobrien
Copy link

Fixes #11204

The change introduced here: 1a3dde2#diff-1b9682e6b936bfe3101e35ff6e574957R61 introduced this issue because it assumed that a falsey obj always meant it was not present. However if obj is a string it can be both falsey and present.

@@ -62,7 +62,7 @@ export function get(obj, keyName) {
Ember.assert(`Cannot call get with ${keyName} key.`, !!keyName);
Ember.assert(`Cannot call get with '${keyName}' on an undefined object.`, obj !== undefined);

if (!obj) {
if (!obj && typeof obj !== 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

I think an isNone (via import isNone from 'ember-metal/is_none';) check here would be better than specifically checking for strings.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, yes that's much neater.

@paddyobrien paddyobrien force-pushed the paddy/get-empty-string-length branch 3 times, most recently from ece32a5 to 0e7ac9c Compare May 18, 2015 15:13
@rwjblue
Copy link
Member

rwjblue commented May 18, 2015

Looks good.

@mixonic / @stefanpenner / @krisselden - Would one of y'all mind reviewing also?

@stefanpenner
Copy link
Member

looks ok, is this a regression? This seems like it has been this behavior for some time.

@paddyobrien
Copy link
Author

It looks to be a regression in 1.12, see http://emberjs.jsbin.com/peposiconi/1/edit?html,js,console,output

@rwjblue
Copy link
Member

rwjblue commented May 18, 2015

@stefanpenner - This was introduced by #3852 which initially landed in 1.12 (it sat as a PR for a very long time). The behavior prior to #3852 was to check obj === null (instead of !obj), this was changed so that it also applied to undefined as of that PR.

Short version: yes, it is a regression in 1.12 that was not present in prior versions.

@paddyobrien
Copy link
Author

Oh also, should this be [BUGFIX release], I found this while working on a 1.12 upgrade and this behaviour would prevent me from continuing that upgrade.

@rwjblue
Copy link
Member

rwjblue commented May 18, 2015

[BUGFIX release] seems fine, we have a handful of other changes for 1.12.1 also.

@paddyobrien paddyobrien force-pushed the paddy/get-empty-string-length branch from 0e7ac9c to 0dab9aa Compare May 18, 2015 16:39
@paddyobrien
Copy link
Author

ok, updated to [BUGFIX release]

@paddyobrien paddyobrien changed the title [BUGFIX beta] Fix get with empty string [BUGFIX release] Fix get with empty string May 18, 2015
@paddyobrien paddyobrien force-pushed the paddy/get-empty-string-length branch from 0dab9aa to 824cac8 Compare May 18, 2015 16:52
@paddyobrien paddyobrien force-pushed the paddy/get-empty-string-length branch from 824cac8 to 05519f7 Compare May 18, 2015 17:35
@stefanpenner
Copy link
Member

Short version: yes, it is a regression in 1.12 that was not present in prior versions.

👍

rwjblue added a commit that referenced this pull request May 18, 2015
@rwjblue rwjblue merged commit 76f5665 into emberjs:master May 18, 2015
@rwjblue
Copy link
Member

rwjblue commented May 18, 2015

@paddyobrien - Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String.length returns undefined for empty string in 1.12
3 participants