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

Method set in simple paths #60

Closed
xgbuils opened this issue Dec 14, 2015 · 6 comments
Closed

Method set in simple paths #60

xgbuils opened this issue Dec 14, 2015 · 6 comments

Comments

@xgbuils
Copy link

xgbuils commented Dec 14, 2015

When it is passed a simple path to set method I expect to have the same behaviour of expression:

obj[path] = value

Examples:

var obj = {}
objectPath.set(obj, 3, 'foo') // now obj is {3: 'foo'}
// or
var obj = {}
obj[3] = 'foo'
var obj = {}
objectPath.set(obj, 'a', 'foo') // now obj is {a: 'foo'}
// or
var obj = {}
obj['a'] = 'foo' // now obj is {a: 'foo'}

But exist in set method one case that it behaves different:

var obj = {}
objectPath.set(obj, '', 'foo') // now obj is {}
// however
var obj = {}
obj[''] = 'foo' // now obj is {'': 'foo'}

I think that it would be a good idea change behaviour in this case to keep the consistency. I think the same for another methods.

@pocesar
Copy link
Collaborator

pocesar commented Dec 15, 2015

yes, because right now it considers '' as empty (along with false, null, undefined, 0, etc). if the path is empty, all functions return prematurely without trying further. although there isn't a use case where you'd want to set an empty string field on an object, can you show me one case that would be useful?

@xgbuils
Copy link
Author

xgbuils commented Dec 15, 2015

I don't know if there are some case that is useful. Only it seems nice for me because keep behaviour expected. And it's posible to implement easily and generally for all methods using this function:

function pathNormalizer (path) {
    if (isArray(path)) {
        return path;
    } else if (isString(path) && path !== '') {
        return parseToArray(path);
    } else {
        return [path]
    }
}

function set(obj, path, value) {
    path = pathNormalizer(path)
    // rest of code
}

function get(obj, path, defaultValue) {
    path = pathNormalizer(path)
    // rest of code
}

/* ... */

var obj = {}
objectPath.set(obj, undefined, 'foo')
objectPath.get(obj, undefined) // 'foo'
objectPath.set(obj, '', 'bar')
objectPath.get(obj, '') // 'bar'

@pocesar
Copy link
Collaborator

pocesar commented Dec 15, 2015

js objects right now can have just 2 types of keys: strings and Symbols. undefined isn't a valid key, it's toString()'d to become obj['undefined'], the same with null, and non-string keys. the closed you could get to setting obj[undefined] is to use obj[Symbol.for('undefined')]

@xgbuils
Copy link
Author

xgbuils commented Dec 19, 2015

Hi.

I checked this behaviour:

var obj = {};
obj[undefined] = 'foo';
obj[undefined]; // 'foo'
obj['undefined']; // 'foo'
obj; // {undefined: 'foo'}

and

var obj = {};
obj[null] = 'foo';
obj[null]; // 'foo'
obj['null']; // 'foo'
obj; // {null: 'foo'}

in Firefox 42, Chorme 46 and node 0.12.0 and the behaviour is valid and does not throw any exception. Then I checked es6 specs and according to specifications it is the correct behaviour. In fact, key can be a Symbol, String or another type value(object, number, boolean, null, etc.) that it will be coerced to string based on specs.

On other hand, I understand that this module aims to extend behaviour to get and set values deeply (a part of another methods). Therefore it is needed an array of keys to get or set values. For example, if you use this sentence objectPath.get(obj, ['foo', 'bar', 3]) try to get obj['foo']['bar'][3] value. If ecmascript spec allows to use permissive values why we want to restrict this? If the module aims to extend behaviour of javascript, why we want to limit another behaviour at the same time?

In my point of view, one module should be focused in one thing to do. Another module could approach the issue of restricting keys. For this reason I proposed something similar to pathNormalizer function and I don't understand restrictions about this use cases:

objectPath.get(obj, [undefined, 'foo', 2])
objectPath.set(obj, [''], 'bar')
// etc

@pocesar
Copy link
Collaborator

pocesar commented Dec 20, 2015

indeed it's correct and that's the spec, but what we are trying to achieve is a consistent 'interface' for acessing properties. if by any means you pass an undefined variable (aka var myvar;) to the get/set function, that's usually a programmer mistake and the lib should account for that. passing non-string values other than Symbols (even if they ARE toString()'d) that's kinda sloppy from a programmer perspective. since you are trying to CHECK if a path is undefined / has a value, the path itself should AT LEAST be specified correctly

symbol support on the otherhand is coming to version 1.0, and is much safer then trying to use falsy values as a key, IMHO javascript is permissive when it shouldn't. trying to do obj['some']['deep']['path'] throws, but using non-string keys and coercing them to string is 'ok'

@xgbuils
Copy link
Author

xgbuils commented Dec 23, 2015

Ok, I understand your opinion at almost cases (undefined, null, and non-string keys) because it usually a mistake and really does not restrict any feature. Form example:

objectPath.get(obj, undefined) // it is not valid

but

objectPath.get(obj, 'undefined') // it is valid and behaves like.

But I do not agree about empty strings because restrict a valid behaviour that can not reproduce otherwise. But is only my opinion.

Thanks!

@xgbuils xgbuils closed this as completed Dec 23, 2015
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

No branches or pull requests

2 participants