Should support for functions be removed?
ericf opened this issue · 11 comments
Serializing functions is tricky business that is basically not possible to get 100% correct without doing insane things. It also encourages bad practices of shipping app code over the wire by serializing it in the HTML. If this foot-gun is left it, it needs big scary docs about how this feature ought to be used!
We do want to share functions between client and server somehow, though, right? At the very least, with what we do with templates and internationalization, those functions are wrapped up into their modules, and accessed through a registry to be used.
If we do remove this from express-state, I think it's still useful to discuss an alternative, such as how the template and internationalization registry system works, and how you would be able to create something similar.
@clarle, in principle, express-state is about transitioning the state of the app from server to client, and a serialized function does not carry any state. What's probably the main reason why we want to remove support for them. Effectible, we could expose monads functions per request, but that doesn't mean we should.
Also, keep in mind that this is a very low level component, with a low level api. It doesn't, and shouldn't, care about templates, loader registry or i18n registry, it cares about data that carry the current state of the app per request.
That been said, we are using function serialization for app.yui.ready() and app.yui.use(), and the reason for that is performance. Sending a little bit of a payload to the client side to boot the client side app without the need to wait for a javascript file to be loaded independently. It is true that we could expose the blob with the functions right after {{{state}}} in our layout template, but then we need to find a mechanism for app developers to overrule that blob somehow, something that is already well implemented in express-state.
So, I vote for keeping support for it, but as @ericf said, adding the proper docs explaining in details what are the implications about serializing functions, and adding few guidelines on how and when to use them.
@caridy good points. I think you explained the pros/cons and primary use cases well. What I'll do, for now, is add docs and guidance about only serializing dependency-free, monadic functions that are for advanced configuration (like YUI config feature tests or Express route param formatters).
@clarle my main worry is that people will use express-state as to ship their application code to the client (which would be very brittle) instead of using a module system and loader. Ideally the source code should be static and independent of state/configuration (which is dynamic).
@caridy the reason I wanted to add support for functions to begin with is to support monadic config functions, like feature tests in YUI config or formatters for Express route params. I think code-inlining is a different problem that we want to solve with another package because it will involve modules, loaders, dependency resolution, and could simply deal with the string contents of a files to concat into the response body.
Closing this. We are keeping support for functions.
So this is something I´ve been thinking about for a couple of weeks.
dependency-free, monadic functions
Let's use the right terms: we can safely share functions with the client as long as they don't access anything in an outer scope. Remarkably, this is easy to check statically. I've been playing around with this small tool that Gozala wrote exactly for this use case: https://github.com/gozala/isoscope. Isoscope takes the AST for a function and gives you back a list of all the variables that function is closing over. For example:
var esprima = require("esprima")
var enclosed = require("isoscope/enclosed")
// Parse some code
var form = esprima.parse(String(function fn(a, b) {
console.log(String(a) + b)
}))
// Get a function form we'll be analyzing
var fn = form.body[0]
fn.id
// => { type: "Identifier", name: "fn" }
// Get names of enclosed references
enclosed(fn)
// => [ "console", "String" ]This means express-state could try to parse the function, analyze it and if it contains one or more unknown enclosed variables it could throw an error saying it can't safely expose the function to the client.
This means express-state could try to parse the function, analyze it and if it contains one or more unknown enclosed variables it could throw an error saying it can't safely expose the function to the client.
Sounds like a cool idea. If we do this, we'd only want to do in dev-mode and not in prod because of the runtime cost.
that means express-state will depend on esprima an isoscope, that will be a mistake IMO. We could provide express-state-dev, that they can include in their devDependencies, and require it to add extra verifications when exposing stuff during dev time, but modifying express-state, which is a lightweight pkg today, just to provide this extra check in dev will be a hard to sell.
Yeah true, and the function will throw on the client because it will try to access something in the outer scope which doesn't exist.
@ericf you don't know when it'll throw, which is dangerous part.
How about this… someone could use these tools or some isFunctionSerializable() wrapper which they can pass their function into before trying to expose it.
Making this feature exist outside of express-state to avoid @caridy's concerns.
Yes, if I want to, I can check for myself. I just thought it could be a nice addition to prevent errors.
I think it'd be nice if JS itself had a reflection API for this BTW.