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

Adding additional template variable scopes #2926

Closed
wants to merge 2 commits into from

Conversation

gfoust
Copy link

@gfoust gfoust commented Mar 4, 2016

This is a follow up to #2924. It addresses the issue of exposing session variables to template engines using a more general approach. Note that unlike #2924, this request does not involve exposing scopes explicitly using certain names.

Each response (potentially) has a stack of template variable scopes. Each scope is an object whose properties define the variables to be exposed. A method was added to the response object named addTmplVars. The parameter to this method is a scope to be pushed on the scope stack. Here is an example:

res.addTmplVars({ name: 'gfoust', count: 3 });

The scope is stored directly on the scope stack. Therefore, any changes made to that scope will be visible to the template engine. For example, the following code listing has the same effect as the previous one.

var obj = {};
res.addTmplVars(obj);
obj.name = 'gfoust';
obj.count = 3;

When a template is rendered, the objects on the scope stack are "flattened" into a single object. This "flattening" does not alter the scopes in any way; it merely copies their properties to a new object. In the case of conflicts, variables from scopes higher up in the stack will replace those from scopes lower in the stack.

Prior to this "flattening", the variables of app.locals are effectively added to the bottom of the stack, and variables from res.locals are added to the top of the stack, followed by variables passed to res.render. This augmented stack may be represented as follows:

variables passed to res.render -> res.locals -> scope stack (high to low) -> app.locals

The arrows here represent precedence: variables from scopes on the left of the arrow will override variables from scopes on the right should a conflict occur.

The idea here is that any middleware can expose a scope to the template engine. For example, when using express-session, it could push the session object onto the scope stack, allowing session variables to be directly accessed by the template. Should multiple middlewares push scopes to the stack, the order in which they execute will define precedence.

I used the tmplVars property of the response object to hold the scope stack. I would expect this to be considered a private variable. The property is not created until the first time addTmplVars is called.

@dougwilson
Copy link
Contributor

This is awesome :)! There are a couple nits in there, but my main question, of course, would be how this would work with features of modules like express-session has the regenerate functionality, which generates a branch new req.session object. For example, if express-session does res.addTmplVars(req.session), but then a user calls regenerate, then how can that module remove the old object from the stack and get the new one added (ideally in the same location?).

Or, would the idea be that express-session and other modules would not be calling res.addTmplVars and instead the user would be responsible for setting up this link (and then they can manage when to add it to account for their use of regenerate)?

lib/response.js Outdated
*
* @public
*/
res.addTmplVars = function addTmplVars(options) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably want to think of a better method name than addTmplVars, but I don't have anything on the top of my tongue. Just adding that I don't particularly care for this name, and would love to hear thoughts from other people on it :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no argument here. I just wrote down the first thing that popped into my head.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a place holder, it's good, but yeah, that doesn't track for the long haul.

+1 We'll probably want to think of a better method name than addTmplVars

@gfoust
Copy link
Author

gfoust commented Mar 4, 2016

I personally was expecting that modules would automatically add variable scopes to the response, thereby lessening programmer burden. I have two thoughts regarding the issue you raised.

The first is that a module could make it configurable as to whether or not they exposed variables to the template. This would mean a programmer could prevent the default exposure doing something like this:

app.use(session({ tmplVars: false }));

Then the programmer could manually add variable scopes whenever appropriate---e.g., after regenerating a session.

The second thought is that we could allow modules to "tag" their scopes by providing an id (e.g., the module name). This id could be an optional parameter, maybe something like this:

res.addTmplVars('myId', scopeObj);

If there were two calls using the same id, the later call would replace the earlier call. In this way a session module could automatically update its scope whenever the session was regenerated. As an added bonus, if you ever decided to expose scopes using variable names (as discussed in #2924) you could use the identifiers as the variable names. Thus, I could be sure I was using variable x from my scope by writing myId.x.

@tunniclm
Copy link
Contributor

tunniclm commented Mar 4, 2016

I like the idea of naming scopes. Could lead to a nice api like res.scope('session').myVariable = val where res.scope([name], [object]) (optional name, optional object) returns the scope from the stack, adding it to the stack if it doesn't already exist. Although, that does raise the issue of name clashes.

@gfoust
Copy link
Author

gfoust commented Mar 4, 2016

I like that idea, @tunniclm. It does create a potential for name clashes, but (I would say) not unreasonably so. It should be easy enough for modules to allow the programmer to modify the scope name used by the module if necessary.

Questions that occur to me: Is it required that all scopes have a name? What about app.local, res.local, and variables passed directly to res.render()---do those scopes have names? And, if they have names, should they be accessible via res.scope()?

For my part, I would suggest that all scopes should be required to have a name, and that you choose and reserve names for app.local, res.local, and variables passed to res.render(). I would think those three scopes should be considered special, and therefore not accessible via res.scope(). For example, you should not be able to set a different scope in place of app.local.

@gfoust
Copy link
Author

gfoust commented Mar 11, 2016

I have taken @tunniclm's suggestion and renamed the addTmplVars function to scope. It has three signatures.

res.scope(name)

If no scope with name name exists, it creates a new one with that name and adds it to the end of the scope stack. Returns the scope with name name.

res.scope(name, scope)

If scope with name name already exists, it is removed from the scope stack. The scope scope is added to the end of the scope stack with name name.

res.scope(name, scope, replace)

If replace is truthy and if a scope named name already exists, then scope scope takes the place of the existing scope in the scope stack. If replace is not truthy or if no scope named name exists then behavior is the same as the previous signature.

I use properties res.scopeStack and res.scopeIndex to respectively store the scope stack and to name the scopes. These properties are not created until the first time res.scope() is called.

The scope stack is completely unrelated to both app.locals and res.locals; it is not possible to access these variables through res.scope().

To support the new behavior, I have added another signature for app.render.

app.render(view, localsStack, callback)

This has the usual behavior of app.render(), but the options object passed to the template engine is created by flattening the locals stack on top of app.locals (i.e., the scopes in localsStack are given higher precedence than app.locals). The function res.render() uses this signature, creating localsStack by copying res.scopeStack then appending res.locals and the scope passed to res.render().

The other signatures of app.render() remain unchanged, with one small exception. Previously, calling app.render(view, locals, callback) would look for locals.locals and, if it existed, merge it into the options object to be passed into the template engine. This was used by res.render() to pass in res.locals. It has been removed since it is no longer necessary. If desired, it could be added back in; however, it is not part of the official API, so I was assuming no one would be relying on it.

@gfoust
Copy link
Author

gfoust commented Apr 19, 2016

I'm not really sure of your process here, so I wanted to check in on this. Is this being considered for possible inclusion in a future release? If so, is there anything I can do to help it be approved? Thanks.

@hacksparrow
Copy link
Member

Great effort and implementation. However, my opinion is that the functionality should be implemented at the application level; we should keep Express as simple as possible.

@expressjs/express-tc

@dougwilson
Copy link
Contributor

Alright, this is definitely a stale pull request. Not sure why we didn't close it, but above is our thoughts, ultimately.

@dougwilson dougwilson closed this Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants