franciscop / server

:desktop_computer: Simple and powerful server for Node.js
https://serverjs.io/
MIT License
3.56k stars 170 forks source link

How to create, set a name and send a file dynamically create #130

Closed cglacet closed 3 years ago

cglacet commented 3 years ago

Context

This not a real issue. I found the answer on the way of writing it, but I decided to still create this issue in case others may have the same question and to add some remarks:

The question

I'm creating a PDF file on the flight (on each request) but I can't find a way to send it. The server.reply.download function only allows to give the name of the file you want to download. In my case, I have no file name and I would like to avoid creating dealing with file system.

Is there any way to do this? Also that would be nice to allow giving a name to the file when downloaded by the client.

cglacet commented 3 years ago

The solution seems to not be available directly via the API proposed. On the other hand I noticed that express what used internally and I managed to find a solution accessing the express instance directly:

async function downloadPDFAs(pdf, fileName){
    context.res.setHeader('Content-Disposition', `filename="${fileName}"`);
    return type('application/pdf').send(new Buffer(pdf));
}
franciscop commented 3 years ago

Hi @cglacet , thank you so much for bringing this up and creating an issue! These kind of issues are great to make the examples collection more complete, please feel free to bring up any similar future issue like this even before you solve it if you want some help.

I just added an example on how to do this with server; basically you got it right, and there's a slightly shorter version of setting the headers without relying in the underlying express implementation:

const { header } = server.reply;

const downloadPdf = (data, name) => {
  return header({ "Content-Disposition": `filename="${name}"` })
    .type("application/pdf")
    .send(new Buffer(data));
};

Or this if you want to prompt a download (note the attachment; in the header):

const { header } = server.reply;

const downloadPdf = (data, name) => {
  return header({ "Content-Disposition": `attachment;filename="${name}"` })
    .type("application/pdf")
    .send(new Buffer(data));
};

There was some documentation on the header() section that combined with the chainable explanation could lead you here, but that surely isn't intuitive! So I also added the bit on how to chain the header() reply based on this issue. So double thank you!

cglacet commented 3 years ago

Thanks for the cleaner version (that's better because if you happen to not use express anymore that will still work). In my case I wanted to have it without the download prompt.

I think I started from the download implementation (in your source code), then I found that using send + Buffer was a solution. I struggled understanding the ctx.res (I had no idea where res came from and where it was documented).

The header part only came later I found the answer by googling node js server change filename send buffer and I noticing that express, which I don't know either, looked a lot like what I saw in your code. I had a bit of luck this time.

BTW, I used your server for helping some friends of mine filling their "lockout certificates" which we need to fill every time we go out in France. It's hosted on a free Heroku instance here. Your server code have helped to create 550 certificates in the last 24 hours 👍 (these are not all my friends but friends have friends and you know, everyone is lazy 😄).

PS, I wanted to check if it was possible to add a design I like to your server, but apparently its not that easy and I'm not an expert in JS, far from it. It's used by a python web framework called FastAPI and it looks a bit like this:

from fastapi import FastAPI

app = FastAPI()

@app.get("/")
def read_root():
    return {"Hello": "World"}

I like having the route right next to the function name, but it seems javascript doesn't provide such easy access to decorators. I think you could find some good inspiration in FastAPI design it's extremely popular, even more popular than the framework it's based on (starlette).

I'm saying that because I can clearly see that you care very much about your documentation. I know how hard it is to end up with a good documentation like yours.

Cheers :)

franciscop commented 3 years ago

Hey thanks again for such an in-depth answer! That website looks great and I'm happy you found server useful.

In python is this:

@app.get("/")
def read_root():
    return {"Hello": "World"}

I considered something like this for Javascript:

server.get('/', ctx => {
  return { hello: 'world' };
});

But then that makes it non-intuitive for the options. Maybe something like this actually makes more sense:

const app = server(options);
app.get('/', () => {
  ...
});

I still find that this has the problem that if you have many routes and want to split them into different files, now you have to do tricks for doing so. Currently if you want to split e.g. userHandler into a different place, you can do it neatly like:

import userHandler from './userHandler';

server(
  options,
  userHandler,
  ...
);

This seems to be your repo, right? Looks good, I have a small couple of tips:

If you define the option views to be { port: 8080, log: 'notice', views: 'templates' }, then you can render it just like this without needing to pass the full path:

return render('build-url.hbs', options);

If you upgrade server to the version 1.0.31, the partials are automatically imported into hbs without needing to do the hbs.registerPartials manually. Then you can write your build-url.hbs like this:

{{> partials/header }}
{{> partials/form }}
etc

This is a fix I did recently, and I also published a small example for that.

cglacet commented 3 years ago

In python, the following:

@decorator
def f(x):
    return 2 * x

Is only a shortcut for:

def f(x):
    return 2 * x

f = decorator(f)

So, in a sense, yes its the same as:

decorator(x => 2 * x)

On the other hand having anonymous, ie. lambdas, functions maybe is more painful to debug?

But then that makes it non-intuitive for the options. Maybe something like this actually makes more sense:

const app = server(options);
app.get('/', () => {...});

An alternative they offer for fastAPI is this (which avoids having a very complex constructor):

app = FastAPI()
app.add_middleware(HTTPSRedirectMiddleware)

I personally find both very similar as a user. But what you suggest is implemented in fastAPI, you can see all the options the app can take here.

I still find that this has the problem that if you have many routes and want to split them into different files, now you have to do tricks for doing so. Currently if you want to split e.g. userHandler into a different place, you can do it neatly like:

Having both options offers more flexibility to your users. In fastAPI you can also add routes directly via the constructor too. I think the best option depends on the context so offering both makes everyone happy. I usually write services that have few endpoints so I tend to use the decorator option. But for larger APIs I'm certain the other option is, like you said, more convenient.

Thanks a lot for you advices, I'll make all these changes 😄

PS, Concerning all design choices I can only guess, maybe you should talk to the creator of fastAPI, he is quite active and usually takes support/questions quite seriously. You also have the gitter which is quite active.

cglacet commented 3 years ago

I haven't completely answered your question on decorators.

@decorator("foo")
def f(x): 
    return 2 * x 

Is actually the same as:

def f(x): 
    return 2 * x 

f = decorator("foor")(f)

Which is not exactly similar to:

decorator("foo", x => 2 * x)

But I think currying the decorator only makes sense because of how it's used in python (with @). In your case I feel it wouldn't makes much sense to offer this interface:

decorator("foo")(x => 2 * x)

So your proposition was the best I could think of using my current understanding of javascript.

EDIT I found this which seems to confirm there isn't a better option: Decorators on functions

cglacet commented 3 years ago

Would something like this be possible? (I added fake get and post to make this a minimal working example):

function get(x, y){
    return `GET ${x} -> ${y}`;
}

function post(x, y){
    return `POST ${x} -> ${y}`;
}

function App(){
    const routes = []
    return {
        get: (...args) => routes.push(get(...args)),
        post: (...args) => routes.push(post(...args)),
        routes,
    }
}

const app = App();

app.get("/", "hello world");
app.post("/test", "some data");

console.log(app.routes);
// [ 'GET / -> hello world', 'POST /test -> some data' ]
cglacet commented 3 years ago

I tried this on my small toy example and it looks like this (complete code example I have tested here : index, app):

function App(options, initialRoutes = []){
    const routes = [...initialRoutes]
    return {
        get: (...args) => routes.push(get(...args)),
        post: (...args) => routes.push(post(...args)),
        server: server(options, routes),
    }
}

const serverOptions = { 
    port: 8080, 
    log: 'notice', 
    views: 'templates',
};

const initialRoutes = [
    get('/get-short', certificateDownload),
]

const app = App(serverOptions, initialRoutes);

async function certificateDownload(context){
    try{
        const profile = getProfile(context.query.id, context.query.delay);
        const reasons = context.query.reasons.split(',');
        return await downloadPDF(profile, reasons);
    }
    catch (error){ 
        return {'error': `${error}`};
    }
}

app.get('/get',
    async function certificateNoConfig(context){
        incrementCounter('get');
        const json = context.query;
        let {reasons, delay, ...person} = json;
        if (!reasons || reasons.length == 0){
            reasons = ['travail']
        }
        else {
            reasons = json.reasons.split(',');
        }
        const profile = computeProfile(json, delay);
        context.res.setHeader('Content-Disposition', `filename="${pdfName(profile)}"`);
        return await downloadPDF(profile, reasons);
    }
);

I havent used the lambda because I still have no idea if its a problem to debug with lambdas. What do you think of this?

franciscop commented 3 years ago

So I did consider that and other designs, but decided to go with the current one. You are free to do a wrapper like that, but I think it's valuable to have a single interface to document and use and that having two very different interfaces would reduce and not increase value for this library. I don't think there's a single "ideal" design here, so I had to pick and I find the current design to scale up better with many more routes.

Thanks for all of that feedback though! I believe the next big thing to do with this library is to reorganize the source code to be more coherent, and migrate it to ES modules, but that'd require releasing v2 and me working a couple of months fulltime on this project and don't have that kind of time right now. I might have in the mid-term (1-2 years), but until then I consider (and found it to be quite true!) v1 to be quite stable and working well.