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

response.sendFile #288

Closed
ghost opened this issue Sep 11, 2017 · 20 comments
Closed

response.sendFile #288

ghost opened this issue Sep 11, 2017 · 20 comments
Labels
type: fix Issues describing a broken feature.

Comments

@ghost
Copy link

ghost commented Sep 11, 2017

If I use response.sendFile("/file") I get the following error:

[NODE] Error
[NODE] at NotFoundError.HttpError [as constructor] (/app/src/http-error/HttpError.ts:19:22)
[NODE] at new NotFoundError (/app/src/http-error/NotFoundError.ts:10:9)
[NODE] at ExpressDriver.handleSuccess (/app/src/driver/express/ExpressDriver.ts:286:35)
[NODE] at /app/src/RoutingControllers.ts:161:45
[NODE] at propagateAslWrapper (/app/node_modules/async-listener/index.js:468:23)
[NODE] at /app/node_modules/async-listener/glue.js:188:31
[NODE] at /app/node_modules/async-listener/index.js:505:70
[NODE] at /app/node_modules/async-listener/glue.js:188:31
[NODE] at
[NODE] Error: Can't set headers after they are sent.
[NODE] at SendStream.headersAlreadySent (/app/node_modules/send/index.js:402:13)
[NODE] at SendStream.send (/app/node_modules/send/index.js:625:10)
[NODE] at onstat (/app/node_modules/send/index.js:737:10)
[NODE] at /app/node_modules/async-listener/glue.js:188:31
[NODE] at FSReqWrap.oncomplete (fs.js:153:5)

The file exists and is readable

@NoNameProvided NoNameProvided added the type: fix Issues describing a broken feature. label Sep 11, 2017
@NoNameProvided
Copy link
Member

Try to use return response.sendFile("/file")

@MichalLytek
Copy link
Contributor

You have to wait for #286.

@ghost
Copy link
Author

ghost commented Sep 12, 2017

If I use this code

 public async getImage(@Res() response: express.Response, @Params() params) {
        try {
            let filePath = path.join(path.normalize(ImagesFolder), params.path);
            response.type("jpg").send();
        } catch (err) {

        }
    }

I get the error

[NODE] Error: Can't set headers after they are sent.
[NODE] at validateHeader (_http_outgoing.js:489:11)
[NODE] at ServerResponse.setHeader (_http_outgoing.js:496:3)
[NODE] at ServerResponse.header (/app/node_modules/express/lib/response.js:730:10)
[NODE] at ServerResponse.send (/app/node_modules/express/lib/response.js:170:12)
[NODE] at ServerResponse.json (/app/node_modules/express/lib/response.js:256:15)
[NODE] at ExpressDriver.handleError (/src/driver/express/ExpressDriver.ts:348:26)
[NODE] at /src/RoutingControllers.ts:129:32
[NODE] at propagateAslWrapper (/app/node_modules/async-listener/index.js:468:23)
[NODE] at /app/node_modules/async-listener/glue.js:188:31
[NODE] at proxyWrapper (/app/node_modules/async-listener/index.js:477:29)
[NODE] at /app/node_modules/async-listener/index.js:505:70
[NODE] at /app/node_modules/async-listener/glue.js:188:31

Is this also caused by the issue that gets fixed in #286?

@MichalLytek
Copy link
Contributor

Yes, routing-controllers is trying to send the response even if you already send it using res object.

@ghost
Copy link
Author

ghost commented Sep 12, 2017

Is there a ETA for this PR?

@MichalLytek
Copy link
Contributor

@NoNameProvided and @pleerock have to review this PR, then it will be released with other merged PRs.

For now you can compile the npm package by yourself.

@pleerock
Copy link
Contributor

@19majkel94 can it be closed now?

@gasperthegracner
Copy link

gasperthegracner commented Sep 26, 2017

I've updated to: [email protected] and this is my code:

 @Get('/File/:name')
  async GetFile(@Req() request: Request, @Res() response: Response) {
    let filePath = path.join(__dirname, '../public', request.params.name);
    console.log('File: ', filePath);
    return response.sendFile(filePath);
  }

I get error at NotFoundError.HttpError [as constructor] ...
I checked that file exists and has right permissions.

@MichalLytek
Copy link
Contributor

You need to return response, not void, which is the result of calling sendFile.

    sendFile(path: string): void;
    sendFile(path: string, options: any): void;
    sendFile(path: string, fn: Errback): void;
    sendFile(path: string, options: any, fn: Errback): void;

@aasailan
Copy link

aasailan commented Oct 2, 2017

so how can solve it

@MichalLytek
Copy link
Contributor

@Get('/File/:name')
async GetFile(@Req() request: Request, @Res() response: Response) {
    let filePath = path.join(__dirname, '../public', request.params.name);
    console.log('File: ', filePath);
    response.sendFile(filePath);
    return response;
}

@aasailan
Copy link

aasailan commented Oct 3, 2017

@19majkel94 I use your code,but it do not work, the framework still try to callback my CustomErrorHandler, which is implements ExpressErrorMiddlewareInterface。so I use Response.headersSent in my CustomErrorHandler to judge whether the Response has been sent. the code look like this;

export class CustomErrorHandler implements ExpressErrorMiddlewareInterface {
	error(error: any, req: Request, res: Response) {
		// if (res.statusCode === 200) return;
		if (res.headersSent) return;
		res.status(500);
		res.json({
			status: 'fail',
			error: error.message
		});
	}
}

@Middleware({ type: 'after' })
export class CustomNotFoundHandler implements ExpressMiddlewareInterface {
	use(req: Request, res: Response) {
		// if (res.statusCode === 200) return;
		if (res.headersSent) return;
		res.status(404);
		res.json({
			status: 'fail',
			error: 'Not Found'
		});
	}
}

@MichalLytek
Copy link
Contributor

so how can solve it

I don't know what "it" is, please open new issue and describe your problem with code samples.

@aigoncharov
Copy link

@aasailan sendFile basically opens a stream. You should await on it and return response only then.

@Controller()
export class ServeSPAController {

  @Get('*')
  public async serveSpa (@Res() res: Response): Promise<any> {
    const fileName = path.resolve(__dirname, '../../../..', staticPath, 'index.html')
    await promisify<string, void>(res.sendFile.bind(res))(fileName)
    return res
  }
}

It would be better to promisify the prototype of Response but I'm not sure how to do that in this case

@MichalLytek
Copy link
Contributor

@keenondrums Right, I was just showing and example of returning response object, not a working solution 😉

For cases like serving spa's index.html it's better to use serve-static rather than custom routes with sendFile.

@aigoncharov
Copy link

aigoncharov commented Oct 21, 2017

@19majkel94 you can't use only serve static. You have to match every route except your api routes to index html. I combine them to serve other static files like js with static middleware and add serveSpa route to serve index.html

@MichalLytek
Copy link
Contributor

sendFile is not for serving SPA index.html. If you really can't use webpack-dev-server/nginx, please don't reinvent the wheel:
https://github.com/bripkens/connect-history-api-fallback/tree/master/examples/static-files-and-index-rewrite
Just register the history with static after create/use ExpressServer call, so all not matched routes can be handled.

@aigoncharov
Copy link

@19majkel94 works lie a charmm. Thnx for the info!

@dashtom3
Copy link

#376 solved the problem and we have to return response (not return ;)to avoid the double return problem: ' Can't set headers after they are sent '.

@typestack typestack locked as resolved and limited conversation to collaborators Aug 9, 2020
@NoNameProvided
Copy link
Member

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: fix Issues describing a broken feature.
Development

No branches or pull requests

7 participants