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

Tip #10 delusion #62

Closed
ihorskyi opened this issue Jan 11, 2016 · 18 comments
Closed

Tip #10 delusion #62

ihorskyi opened this issue Jan 11, 2016 · 18 comments

Comments

@ihorskyi
Copy link

In example you have this object

var myObject = {
  name: '@tips_js'
};

and this check

if (typeof myObject['name'] !== 'undefined') { ... }

and then these words saying that that kind of check is ok.

Thats ok, but you have to know that there are two native methods for this kind of thing

Actually it is not ok, consider this example:

var myObject = {
  name: '@tips_js'
}

console.log(typeof myObject['name'] === 'undefined') // false
myObject.name = undefined
console.log(typeof myObject['name'] === 'undefined') // true

Which clearly shows that typeof myObject['name'] === 'undefined' is not checking existence of property, but checks type of that property value

@rocketinventor
Copy link

@igor-galchevsky In practice, a variable (or property) that exists should not be set to undefined so what difference does it make?
undefined_js
If it is set as undefined, it was probably suppose to not exist anyways, right?

@ihorskyi
Copy link
Author

@rocketinventor, in practice I saw lot of people with more than 3 years JS experience setting undefined to property meaning it is empty. And also tons of other mistakes that should not be done.

So if you want to say it is a right way to assign null to param when you want to say it is empty - that's correct, if you want to say it is a good practice - also correct, but saying:

  1. You shall never expect in practice someone assign undefined meaning property is empty
  2. typeof myObject['name'] === 'undefined' is ok to check that property exist

is completely wrong, especially when inexperienced rookies reading these JS tips and have at least few places in their code where they assign undefined

@mynamesleon
Copy link

@igor-galchevsky: you have a valid point, but the problem you've raised also applies to the other mentioned methods.
E.g.

var o = {
    test: undefined
};
typeof o.test === 'undefined'; // true
o.hasOwnProperty('test'); // true
'test' in o; // true

If you look at the ECMAScript source code, the hasOwnProperty method is ultimately checking for undefined.

I will add to this point though, that it is often agreed that native JavaScript methods are slower than doing the check yourself. There's quite a lot of extra handling that goes on in the hasOwnProperty method that isn't needed if you're just checking the property against undefined.

@loverajoel: A small point that may be worth mentioning about this as well. If you use hasOwnProperty on the window in IE7, it errors.

@rocketinventor
Copy link

Why don't we write up a tip for this, then?
Obviously, this is something people need to know.
Also, @igor-galchevsky When is this a problem?

@ihorskyi
Copy link
Author

@rocketinventor, what do you mean "when" ? :)

@dfkaye
Copy link

dfkaye commented Jan 12, 2016

@loverajoel
Copy link
Owner

I think that the discussion here is if people define empty vars as null or undefined.
The mistake is set undefined an empty var, if you don't do it, the expression will work.
Maybe we can fix the tip and add a message indicating that common mistake or you can send another tip talking about how to set empty vars ✋
What do you think?

Thanks for this rich discussion 😄

@FagnerMartinsBrack
Copy link

It is definitely not ok to use typeof myObject['name'] !== 'undefined' to check if a property is present in an object.

Regarding setting empty references:

  • typeof is broken.
  • JavaScript uses null for functions that should return a single element contract but returns nothing instead (document.getElementById('')).
  • JavaScript uses undefined for values or properties that are not defined (document.getNothing).
  • undefined can be better minified by assigning into an undefined variable in scope, so it makes sense using with the same semantic as null in certain circumstances to save bytes:
(function( undefined ) {}()) -> (function( a ) {}())
  • Imperative definition of when to use null or undefined enters the "space vs tabs" domain of bikeshedding and has no value as a tip because it depends upon context and project specific tradeoffs.

@loverajoel
Copy link
Owner

@FagnerMartinsBrack I'm not agree with your last point and why do you says that typeof is broken for empty references?

I will make a pr and remove this check and add a link to this discussion.

@dfkaye
Copy link

dfkaye commented Jan 13, 2016

If I understand the complaint, it's that typeof o['name'] !== 'undefined' is not a proper existence check, but a value check disguised as a type check. If you define something as null, its type will be 'object'

function test(actual) { console.log(!!actual); }
function undef() { /* returns undefined */ }
var o = {};

// existence checks
test(typeof o.key === 'undefined'); // true
test(o.key == null); // true
test('key' in o); // <= false - the key of 'key' is not declared

// exists but undefined
o.key = undef();
test(typeof o.key === 'undefined'); // still true
test(o.key == null); // still true
test('key' in o); // true, since o.key has now been declared even if it has no value

// no longer exists
delete o.key; // remove key from o
test(typeof o.key === 'undefined'); // still true
test(o.key == null); // still true
test('key' in o); // false again, since 'key' is no longer a property name in o

// exists but defined as null...
o.key = null;
test(typeof o.key === 'undefined'); // <= false
test(typeof o.key === 'object'); // <= true
test(o.key == null); // still true
test('key' in o); // true again, since o.key has now been declared even if it has no value

@loverajoel
Copy link
Owner

fixed d885967

@ihorskyi
Copy link
Author

@dfkaye that's right. My point is that it is just unsafe to check property existence using typeof myObject['name'] !== 'undefined'.

@dfkaye
Copy link

dfkaye commented Jan 13, 2016

@loverajoel @igor-galchevsky
Ah I see now ~ the tip text is really about checking whether an object property is non-null/undefined
That check should be

if (o.propertyName != null) {...}

@FagnerMartinsBrack
Copy link

@loverajoel

I'm not agree with your last point

The previous bullet point make it clear that if you care for bytes you can use undefined for not defined values semantic, but if you don't care then you don't need to and you can use null. Again, it depends upon the context of what you are doing.

typeof is broken for empty references?

It depends on what you consider "empty references".

"If something works sometimes, and sometimes it does not, and there is a better option, always use the better option". If one does not agree with this principle, then there is no point to continue the discussion.

typeof rules are complicated for some values and useless for others (like checking if a property exists).

When you say typeof myObject.name do you expect the value 'undefined' or that there is no property? There is no way to tell because it only returns 'undefined' either way. The only way to consistently check if a property is in an object (which is what tip 10 refers to), is using obj,hasOwnProperty:

var obj = { a: undefined };
obj.hasOwnProperty( "a" ); // true
typeof obj.a; // undefined

var obj = {};
obj.hasOwnProperty( "a" ); // false
typeof obj.a; // undefined

See that both typeof returns undefined, a falsey value, but only hasOwnProperty makes it right, returning a false (not falsey) value.

@FagnerMartinsBrack
Copy link

@loverajoel

fixed d885967

If myObject has a property with value 0 it will return falsey and not enter the condition, therefore it is also not ok to check if a property is present with the if (myObject['name']) code.

@z-vr
Copy link

z-vr commented Jan 14, 2016

Is it not easier to just do
if(test.key){ console.log(test.key); }

@indianakernick
Copy link

Why don't you use hasOwnProperty like @FagnerMartinsBrack said! It's simplest and most obvious solution. Using typeof to check existence is silly

@zenopopovici
Copy link
Collaborator

Hey @Kerndog73, @FagnerMartinsBrack, can you guys submit a PR with the required changes then we can review it and merge it.

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

No branches or pull requests

9 participants