Allow access to prototype properties

[fredludlow]

Hi there, thanks for a great library! I've got a small PR, though it's entirely possible the current behaviour is by design (in which case I can work around it)

Basically, this works as expected

sprintf("%(x)s", { x: 2 })
// "2"

While this doesn't (or at least, not how I'd expect ;) )

class C { get x () { return 2 } }
sprintf("%(x)s", new C())
// Error: [sprintf] property "x" does not exist

The PR replaces !hasOwnProperty check with a === undefined check and adds a test. It doesn't break any other tests but I guess it's possible someone's relying on an error here?

[alexei]

Well, now that I think of it - 💩

prop in obj might not be the best idea if prop is only a setter. It returns true, though obj.prop would be undefined

[fredludlow]

Okay, hadn't really thought about intentional values of undefined. While playing around I also noticed that currently sprintf("%(x.y)s", { x: undefined }) throws a TypeError.

So, does this sound reasonable.. Requirements are: 1) Allow undefined as terminal value in a lookup (e.g as above. sprintf("%(x.y)s", { x: { y: undefined } }) is okay and should render 'undefined' 2) Avoid undefined in intermediate values (e.g. {x: undefined} not okay in the above example) 3) Detect write-only properties?

Not sure how to do 3! 1 and 2 could be done by breaking out the last run through the loop

for (k = 0; k < ph.keys.length-1; k++) {
    if (arg[ph.keys[k]] === undefined) { 
        // Throw error
    }
    arg = arg[ph.keys[k]] // arg is definitely defined (req 2)
}
var key = ph.key[ph.keys.length-1]
if !(key in arg) // Throw here?? 
arg = arg[key] // arg might be explicit undefined (req 1)

[fredludlow]

Apologies if this is now on a tangent - if the purpose of the hasOwnProperty check is to provide useful errors (rather than an unhelpful TypeError) then maybe just try/catch/rethrow around the arg[key] access?

[alexei]

Yes, you got it right. !(ph.keys[k] in arg) should solve 1 & 2, not 3, but we can just forget about 3. sprintf('%(foo)s', {set foo(val) { this._foo = val }}) will yield undefined, which is not necessarily incorrect. If one deems it incorrect, they'll either fix their code, or complain here.

So:

sprintf('%(foo)s', {set foo(val) { this._foo = val }}) // undefined
sprintf('%(a.b.c)s', {a: {b: {get c() { return 42 }}}}) // 42
sprintf('%(x.y)s', {x: {}}) // raises

But:

sprintf('%(x.y)s', {x: undefined})

will eventually raise TypeError: Cannot use 'in' operator to search for 'y' in undefined I'm fine with that.

[alexei]

Yes, that's correct, the purpose of that condition is to provide a helpful error message. A try/catch block would only work when prop is undefined. undefined['foo'] raises, 42['foo'] does not.

---

Later edit: Yes, that's correct, the purpose of that condition is to provide a helpful error message. A try/catch block would only work when obj is undefined: undefined['foo'] raises, 42['foo'] does not.

[alexei]

So there are basically two options: either drop the condition and leave it to the user to figure out what went wrong, or [try to] implement a method that determines whether a variable has a certain property or not. As if that weren't enough, 'foo'.hasOwnProperty('length') is true, but 'length' in 'foo' obiously raises.

[alexei]

How horrific would it be to use something like

function has_property(obj, prop) {
    return (typeof obj == 'object' && prop in obj) || obj.hasOwnProperty(prop)
}

?

[fredludlow]

Hmm, I've gone round in circles with my thinking a few times...

What's wrong with simply checking if arg is undefined?

for (k = 0; k < ph.keys.length; k++) {
    if (arg == undefined) { // == to capture both undefined or null 
        throw new Error("helpful message") // We have the full keys array to help us here
    }
    arg = arg[ph.keys[k]] // Question: will this fail for any arg other than undefined and null?
}

[alexei]

Yes, it's so simple, it's great 👍

[fredludlow]

I think that CI error was a one-off with the build system (it didn't get as far as actually running the tests)

[fredludlow]

@alexei - I think the 02120d4 addresses your requested changes, but let me know if there's anything else to do

[fredludlow]

Great, thanks!

[alexei]

Thanks for your contribution

[fredludlow]

Sorry, one last thing - I'd like to straighten out dependencies in a downstream project. Does a change like this warrant a push to npm or should I keep pointing to a git url for now?

[alexei]

I can release a minor version, but I need to update the changelog and build dist files

[vdiez]

Hi,

I was preparing another PR from my experience with version 1.1.1 when I saw that the behaviour changed because of this commit:

In 1.1.1, when using named arguments we were getting this error if property was not in the passed object:

argv: { '0': '%(root)s\%(type)s\.transcoding\%(clip_id)s%(transcode_extension)s', '1': { transcode_extension: '.mp4' } }

Result with 1.1.1: Error: [sprintf] property "root" does not exist

Now, checking arg against undefined does not throw the exception because the object is not undefined, its property root is. So we get "undefined" in the result string. Is that expected?

Result after this commit:

undefined\undefined.transcoding\undefined.jpeg

with no exception being thrown

[alexei]

Not sure I understand. Can you please provide a test case?

[vdiez]

I wrote it in the message:

string: '%(root)s\%(type)s.transcoding\%(clip_id)s%(transcode_extension)s' replacement fields: { transcode_extension: '.mp4' } sprintf('%(root)s\%(type)s\.transcoding\%(clip_id)s%(transcode_extension)s', { transcode_extension: '.mp4'})

Cheers

[fredludlow]

I see what you mean, the change from this PR basically only throws when you try to access a property of undefined.

I think it comes down to what the expected behaviour is in these cases:

var o1 = { foo: undefined }
var o2 = {}
sprintf('%(foo)s', o1) // 'undefined' or throw?
sprintf('%(foo)s', o2) // 'undefined' or throw?

The old hasOwnProperty check would throw on the second but not the first, current behaviour lets both through

function C1() {}
C1.prototype = { foo: undefined }

function C2() {}
C2.prototype = {}

sprintf('%(foo)s', new C1())
sprintf('%(foo)s', new C2())

Old behaviour would throw on both (so prototype and own properties treated differently)

Maybe a variation on what @alexei suggested above:

if (arg == undefined || !(arg.hasOwnProperty(key) || (arg instanceof Object && key in arg)))

I think this would:

• Never try to access properties of undefined/null
• Allow prototype properties
• Allow explicit nulls/undefined attributes but throw for missing attributes, so @vdiez 's example would throw again I think.

[vdiez]

In fact I totally agree with this permissive behaviour, my PR was going precisely that way not throwing and exception, but instead of setting to undefined to leave the original placeholder in the output, as I have a chain of objects each replacing one or various placeholders at a time. Check my commit vdiez@8bf2f95

edit: vdiez@27c7ce7 better to avoid ignoring falsy values

[vdiez]

Anyway I know this is not the correct place to discuss my PR and I should open my own, but just trying to see what would be the appropiate behavious according to you guys, as I see it changed after this merge.