Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

r.js is broken on server for non-hash objects #102

Closed
coffeemug opened this Issue · 11 comments

4 participants

@coffeemug
Owner

The following works:

r.expr([ { a: 1}, { b: 2 } ]).map(r.js('return this')).run()

However, the following doesn't:

r.expr([ 1, 2 ]).map(r.js('return this')).run()

In the latter case this object always evaluates to {}. I suspect this has to do with the code that invokes v8 assuming a hash rather than an arbitrary object when binding the v8 context.

Could @jdoliner or @mlucy look at this?

@mlucy mlucy was assigned
@mlucy
Collaborator

There's a comment in the code claiming that this in javascript can't be bound to a non-object. @wmrowan -- is that actually true?

@coffeemug
Owner

Errr. I'm not 100% sure that this is the case, but if it is, that's unfortunate... We should still try and do something better though, for example, wrap it in an object with an attribute value or something of the sort. This way it'll be slightly uncomfortable, but at least doable.

@coffeemug
Owner

@mlucy -- are you still on this, or should we reassign to @wmrowan ?

@wmrowan

This is true. Try running the following:

> (function() { return this; }).call(1)
{}

I may have found a way around the problem but it will require more testing to ensure that it doesn't have pernicious cases. I'll take this on since I'm already working on JS evaluation.

@wmrowan wmrowan was assigned
@wmrowan

@coffeemug Even if there is a way to solve this issue, I'd actually be in favor of dropping this altogether in the new spec. All it really saves you from his having to provide a function expression to r.js when supplying JS to evaluate in a mapping.

table.map(r.js(function(row) { return row.value }))

vs.

table.map(r.js("this.value"))

Though yes, the latter is shorter, this isn't really the expected behavior as this is technically supposed to be bound to the receiver object of a method call and there is no obvious method call happening here. Really, this is just part of the general confusion surrounding the implicit variable. Given the syntactic advances we have made with do and the lambda syntax I would say that the implicit variable has been obsolete for a long time. This probably isn't the right place to have this discussion so let's talk about it in person when we're all here.

@coffeemug
Owner

Unfortunately, implicit variable has not been made obsolete in the general case. Having to type function ... in js just to get everyone with age older than 30, for example, unfortunately isn't acceptable.

I talked to @mlucy about this and at the time we thought the best option here is to return a sensible error to the user. Now that I think about it, this will prevent the user from using function syntax here, so perhaps a better solution is to return warnings and just bind contexts to empty objects like we do now.

@wmrowan

So, we're going to forget about the implicit variable and instead replace it with this shortcut:

r.row === lambda x: x
r.row['val'] < 1 === lambda x: x['val'] < 1

Users who want to access the current row from a javascript expression will have to use a javascript function expression.

@mlucy
Collaborator

How is this different from what we have now? r.row looks like the implicit variable to me.

@al3xandru

I'm so glad @mlucy has asked this first as I've been scratching my head for the last few minutes trying to see the difference...

@wmrowan

r.row is explicit. No longer is the shortcut r itself an implicit reference to the current row. By making this clear more people will be inclined to use the helpful shortcut rather than ignore it or puzzle about what it's suppose to mean.

The main difference though is in the implementation. All the complicated hacks surrounding overloading the module name can go away and the new shortcut simply wraps the new way of doing things.

@wmrowan

Since we're not going to fix this now I'll close it. The new behavior will be included as part of the new spec.

@wmrowan wmrowan closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.