Fix route with address '/client/*' not working

[habibrosyad]

Before submitting the PR, please make sure you do the following

• It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
• This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
• Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

• Run the tests tests with npm test or yarn test)

Fix #1142

[benmccann]

It seems a bit funny to me to check if the file exists by trying to read it and then catching the exception. Also, if we're not checking the reason for the exception we might return 404 for issues that should be a 500. Why don't we call fs.existsSync to check if the file exists instead? Maybe something like this:

		if (filter(req)) {
			const file = path.posix.normalize(decodeURIComponent(req.path));
			if (fs.existsSync(file)) {
				const data = read(file);
				const type = mime.getType(req.path);
				res.setHeader('Content-Type', type);
				res.setHeader('Cache-Control', cache_control);
				res.end(data);
				return;
			}
		}
		next();

[Conduitry]

My understanding is that checking whether a file exists before trying to open it is considered a bad practice in general (because of the race conditions this can introduce), and that you should just try to open it and then handle the exception.

I'm not sure what errors that should be a 500 that you'd referring to. The idea of this change (which is what I suggested in a comment on #1142) is to pass control along to the next middleware, which would handle the route normally for URLs that don't match existing files in the built app.

[benmccann]

Ah, I hadn't considered a race condition. I was sort of assuming that files under /client wouldn't be modified will the server was running

I didn't have a specific type of error in mind that might be a 500, but I imagine reading a file might fail under rare circumstances? Permissions, file lock interrupted, file moved or deleted while reading, unsupported encoding, corrupted file, hardware error? Or maybe some user error like putting a directory name in img src? I'm not quite sure and am just brainstorming possible errors, but I would be surprised if we can assume it will never fail. Could we check the exception err.code === 'ENOENT' or err typeof URIError or whatever it is that indicates the type of error?

[habibrosyad]

@benmccann, I've tested with several edge cases that you've mentioned which possibly caused 500:

• permission, in this case EACCES error would be returned
• corrupted file, in this case there are no error returned, I can access the file just fine from the browser but it was of course unreadable
• symlink, this returned just fine
• broken symlink, ENOENT returned in this case, and I think it's valid because it's just the same as file cannot be found in server

I think it is indeed very rare case in that part of code to generate error other than ENOENT. Furthermore, in my understanding all those files under /client/ directory is generated during the build. Therefore, if there are some problems (e.g. permission) it should happen before /client/ directory being populated with files.

But just to make sure, maybe we can go with something like this:

if (err.code === 'ENOENT') {
	next();
} else {
	res.statusCode = 500;
	res.end('something wrong happened');
}

any thought?

[benmccann]

That seems fine to me. I only wonder what should go in the else block for the error case. I'm not that familiar with Sapper's error handling. E.g. if you want to setup something like Sentry that automatically logs all exceptions, I'm not sure the best way to do that with Sapper. Is it better to rethrow the original error in the else or is it better to return 500 there like you have shown? It seems to me like rethrowing the original exception might be more user configurable, but perhaps @antony or @Conduitry have better knowledge in this area than I do

[habibrosyad]

@benmccann, as far as I can tell from reading the Sapper's code, this block is intended to serve static assets only. In this case any assets generated under /client/ (as this particular block will only be executed from this line). Therefore, I somehow could understand why currently the error block set 404 status code directly instead of passing the control to the next middleware.

It's just happen that if a user coincidentally defined a path named /client/ under the routes directory then there will be an overlap with the /client/ path of the generated assets. As it falls into the if (filter(req)) block, therefore instead of processing it as normal route, it is treated as accessing static assets. Hence, the suggestion of @Conduitry to just pass it to the next middleware which will make it handled as a normal route. I don't think rethrowing the error here has any real benefits.

[benmccann]

We might be talking about different else blocks.

I think your suggestion of the following is reasonable:

if (err.code === 'ENOENT') {
	next();
} else {
	res.statusCode = 500;
	res.end('something wrong happened');
}

I was just wondering if it would be better to do:

if (err.code === 'ENOENT') {
	next();
} else {
	throw err;
}

I'm not saying it is better - just wondering to make sure we consider both options and decide which is the proper way to do things

[habibrosyad]

I think I'm going with the first approach for now as I'm also not too familiar with Sapper's error handling and it might become a breaking change if the error is not handled properly later. Can update it later if necessary.

[benmccann]

thanks for this!!

[benmccann]

Closes #1200