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

Marko support #4

Merged
merged 4 commits into from
Apr 5, 2017
Merged

Marko support #4

merged 4 commits into from
Apr 5, 2017

Conversation

delvedor
Copy link
Member

@delvedor delvedor commented Apr 4, 2017

As titled.
The benchmark are very promising, with Marko we gain at least 1000 request per second compared to ejs :)

Feedbacks?
cc @mcollina @jsumners @patrick-steele-idem

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@jsumners
Copy link
Member

jsumners commented Apr 4, 2017

Looks good. Except, I don't see how templates could be modified during development and reloaded without completely restarting the server since they are cached.

@delvedor
Copy link
Member Author

delvedor commented Apr 4, 2017

@jsumners as you can see here the cache system is used only when NODE_ENV is set to 'production' :)

@jsumners
Copy link
Member

jsumners commented Apr 4, 2017

@delvedor ah yes, it's early and I didn't quite parse the way that is working. Looks good.

FYI, and I know it is generic for multiple "engines", but Marko stuff shouldn't be stored in the cache at all. It is already cached by Node's internal require mechanism.

@delvedor
Copy link
Member Author

delvedor commented Apr 4, 2017

@jsumners let me understand, the load api read the template file just one time and then it uses a cache?

@jsumners
Copy link
Member

jsumners commented Apr 4, 2017

@delvedor yes, depending on configuration passed into the Marko engine. In short, once Marko has compiled a template it requires it just like any other module. So the next time a template is referenced, Marko will attempt to require it and Node will return it just like any other module.

@delvedor
Copy link
Member Author

delvedor commented Apr 4, 2017

@jsumners ah ok, but how can marko know when I've changed the original template?

@jsumners
Copy link
Member

jsumners commented Apr 4, 2017

By passing in the right configuration to Marko:

https://github.com/marko-js/marko/blob/ff9334f8893967dea12a8df3927cf9ce62a38bed/compiler/config.js#L31

That should be settable via the options passed in to Marko. But it doesn't seem to be documented in the new docs. I'll have to file a bug report on that one. The old docs are at http://v3.markojs.com/docs/marko/get-started/#hot-reloading-templates

@delvedor
Copy link
Member Author

delvedor commented Apr 4, 2017

@jsumners ok, so your suggestion is do something like this, right? :)

  function viewMarko (page, data, opts) {
    if (!page || !data) {
      this.send(new Error('Missing data'))
      return
    }

    const template = engine.load(join(templatesDir, page))

    if (opts && opts.stream) {
      this.send(template.stream(data))
    } else {
      template.renderToString(data, send(this))
    }

    function send (that) {
      return function _send (err, html) {
        if (err) return that.send(err)
        that.header('Content-Type', 'text/html').send(html)
      }
    }
  }

@patrick-steele-idem any suggestion? :)

@patrick-steele-idem
Copy link

Nice job on fastify! I'm definitely a fan of minimalist server-side web frameworks that are optimized for speed.

Regarding Marko integration, the following pattern is what I would recommend for Marko if we could make it work:

const fastify = require('fastify')()
const template = require('./path/to/template.marko'); // The `.marko` extension is optional, but added here for clarity

fastify.get('/', (req, reply) => {
  template.render(reply.res, { text: 'text' }); // res would be the raw writable HTTP response stream and marko will automatically end the response when it done

  // OR, if we were allowed to monkey-patch the `reply` prototype some how:
  reply.marko(template, { text: 'text' });
})

fastify.listen(3000, err => {
  if (err) throw err
  console.log(`server listening on ${fastify.server.address().port}`)
})

With this approach, the point-of-view library is really not adding much value. I am not a fan of the Express view engine for similar reasons. I had written article on this a long time ago that you might find interesting: Bypassing Express View Rendering for Speed and Modularity

Also, for reference: Express + Marko

Thoughts?

@jsumners
Copy link
Member

jsumners commented Apr 4, 2017

That's basically my original statement @patrick-steele-idem --
fastify/fastify#30 (comment)

@delvedor I was just trying to make you aware of the way Marko works since you are implementing a cache in the plugin. If you think adding special cases for specific engines is acceptable, then your suggestion looks good to me.

@delvedor
Copy link
Member Author

delvedor commented Apr 4, 2017

@patrick-steele-idem thank you! :)

OR, if we were allowed to monkey-patch the reply prototype some how:

decorate reply is very easy via the decorateReply api

With this approach, the point-of-view library is really not adding much value.

Indeed, point-of-view does nothing that you can not do without it, is just a nice wrapper (with almost zero overhead) to make easier the user life (and also add a cache system for the other template engines).
I would like to maintain the view api for clarity, and as you can see here, point-of-view is doing what you suggested with reply.marko(...).
My concern is if I'm doing it in the best way :) (this is the actual proposal)

@jsumners yeah, I was not aware of the marko cache, so in this case lru is useless :)

@delvedor delvedor merged commit dfa89de into master Apr 5, 2017
@delvedor delvedor deleted the marko-support branch April 5, 2017 07:30
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

Successfully merging this pull request may close these issues.

None yet

4 participants