david-mark / My-Library

The original cross-browser JS library/framework
http://www.cinsoft.net/mylib.html
59 stars 14 forks source link

Added `undefined` check to `isHostObjectProperty` #4

Closed Raynos closed 12 years ago

Raynos commented 12 years ago

Checking for undefined is safer then for truthy.

Known issues

isHostObjectProperty(document.body, 'nextElementSibling');

isHostObjectProperty(document.body.childNodes, 'length');

Note: I could not find any unit tests so I did not edit or run them.

Note 2: added undefined to outer IIFE for gaurding againts window.undefined = true and for better minification)

david-mark commented 12 years ago

!== null, not undefined. Look at what the first test does (undefined would short-circuit the conjunction).

On Sun, Jan 22, 2012 at 3:07 PM, Raynos < reply@reply.github.com

wrote:

Checking for undefined is safer then for truthy.

Known issues

isHostObjectProperty(document.body, 'nextElementSibling');

isHostObjectProperty(document.body.childNodes, 'length');

Note: I could not find any unit tests so I did not edit or run them.

Note 2: added undefined to outer IIFE for gaurding againts window.undefined = true and for better minification)

You can merge this Pull Request by running:

git pull https://github.com/Raynos/My-Library patch-2

Or you can view, comment on it, or merge it online at:

https://github.com/cinsoft/My-Library/pull/4

-- Commit Summary --

  • Added undefined check to isHostObjectProperty

-- File Changes --

M mylib.asp (4)

-- Patch Links --

https://github.com/cinsoft/My-Library/pull/4.patch https://github.com/cinsoft/My-Library/pull/4.diff


Reply to this email directly or view it on GitHub: https://github.com/cinsoft/My-Library/pull/4

Raynos commented 12 years ago

This depends on the what the purpose is of isHostObjectProperty

Do we want to check whether it's an object or anything which we might want to get properties of?

if (isHostObjectProperty(x, "prop") && x.prop.anotherProp) { ... }

Or check whether a host object has a property at that name

if (isHostObjectProperty(x, "prop")) { ... }

If it's the latter then it won't work for some properties. For example if (isHostObjectProperty(document.body, "tagName")) returns "string" for the typeof in modern browsers which isn't allowed by the regular expression.

If it's the former then this pull request will be closed.

Raynos commented 12 years ago

As noted in the gist the purpose of the method is to see whether a property of a host object is an object so this pull request breaks behaviour.