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

Passing Context objects through express does not work #743

Closed
adamkdean opened this issue Sep 10, 2016 · 7 comments
Closed

Passing Context objects through express does not work #743

adamkdean opened this issue Sep 10, 2016 · 7 comments

Comments

@adamkdean
Copy link

I've been having an extended discussion on StackOverflow (link) with @sethkinast regarding passing Context objects through Express.

I have put together a very quick example of how to replicate this issue here: https://github.com/adamkdean/dust-test

But to save time, the script is this:

const path = require('path')
const hoffman = require('hoffman')
const express = require('express')
const request = require('request')
const duster = require('duster')
const dust = duster.dust

const app = express()
const base = dust.context({
  isBase: true,
  age: 26,
  people: [
    { name: 'exampleA' },
    { name: 'exampleB' }
  ]
})

app.set('views', path.join(__dirname, 'views'))
app.set('view engine', 'dust')
app.engine('dust', hoffman.__express())

app.get('/', function (req, res) {
  res.render('people', {})
})

app.get('/pojo', function (req, res) {
  res.render('people', {
    age: 26,
    people: [
      { name: 'exampleA' },
      { name: 'exampleB' }
    ]
  })
})

app.get('/context', function (req, res) {
  res.render('people', base.push({
    people: [
      { name: 'adam' }
    ]
  }))
})

app.listen(4000, function () {
  console.log('Visit http://localhost:4000 to see streaming!')
})

If you go to /pojo then the plain old JavaScript object renders fine (the template is {#people}<li>{name} aged {age}</li>{/people}), but if you go to /context then it doesn't render fine at all.

I think this would probably be a better place to discuss the issue.

@sethkinast
Copy link
Contributor

Here's the issue: https://github.com/expressjs/express/blob/master/lib/application.js#L535

A new object, renderOptions, gets created. This object of course doesn't have Context as its prototype so Dust tries to shove it into a context.

It's very interesting that no one has ever run into this before. I would never have expected Express to mangle a context object passed to it. We can fix this by dropping the instanceof check on our end.

@sethkinast
Copy link
Contributor

Hoffman even does the same (IMO incorrect) behavior in their stream method:

    res.stream = function(template, data, cb) {
      // flatten and add app and res locals data
      var options = {},
        stream;
      extend(options, res.app.locals);
      extend(options, res.locals);
      extend(options, data);
      ...

@sethkinast
Copy link
Contributor

While we patch this you could manually stream your templates: https://github.com/linkedin/dustjs/blob/master/examples/streaming/app.js

You'll just have to define dust.onLoad yourself instead of your Express engine doing it for you.

sethkinast added a commit to sethkinast/dustjs that referenced this issue Sep 10, 2016
@adamkdean
Copy link
Author

Thanks Seth, appreciate the responses + effort involved. I thought I was doing something wrong, as like you say, very strange that this is the first time this has been run into. Then again, most examples exclusively use plain objects for their view models so perhaps that's why?

Anyway, thanks again. Will patch my version while I wait for your patch to hit!

@sethkinast
Copy link
Contributor

Thanks for taking the time to report! I'm actually really fascinated that this has never been a problem before. I wonder if Express 4 changed the way that they merge options and locals.

@adamkdean
Copy link
Author

Not a problem at all. Could not for the life of me understand what I was doing wrong. Thanks again, got everything working in my project now and no need for consolidate/hoffman now. 🎉

@sethkinast
Copy link
Contributor

This is available as 2.7.4.

jrrbru pushed a commit to jrrbru/dustjs that referenced this issue Nov 25, 2019
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