Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Investigate alternatives to checking accept header for text/html #45

Closed
ganemone opened this issue Dec 4, 2017 · 6 comments
Closed
Assignees

Comments

@ganemone
Copy link
Contributor

ganemone commented Dec 4, 2017

Currently we check the accept header for text/html in order to decide if the route should be a ssr route or not. This can be confusing in development when someone wants to see the JSON response of some data endpoint and they load the endpoint via the browser URL bar. This can potentially run SSR logic alongside the intended code, which may affect performance metrics and other SSR-specific side effects.

@rtsao
Copy link
Member

rtsao commented Dec 4, 2017

I think enforcing an underscore prefix for non-SSR routes (and enforcing that SSR routes do not have an underscore prefix) would be an acceptable alternative.

I don't see a use case for non-SSR routes and human-friendly URLs.

@KevinGrandon
Copy link
Contributor

Now that we have DI it seems that this may be easier? Perhpas the SSR plugin requires a new token, which can list globs of routes that we ignore the text/html header for. E.g.,

import {DisableServerRoutesToken} from 'fusion-tokens';

// In server:
this.middleware({element: ElementToken, disabledServerRoutes: DisableServerRoutesToken.optional}, ssrPlugin);

@slonoed
Copy link

slonoed commented Feb 28, 2018

I have the same problem. I can't serve files, like images which can be previewed in a browser.

Solution 1: check response header Content-Type (or ctx.type). If it was changed by middleware and the value is not text/html then skip SSR logic.

Solution 2: give a developer a way to prevent SSR explicitly.

Personally, I like the first solution. It is easier to use.

@KevinGrandon
Copy link
Contributor

KevinGrandon commented Mar 1, 2018

Solution 1: check response header Content-Type (or ctx.type). If it was changed by middleware and the value is not text/html then skip SSR logic.

I think this is a clean approach, and I like it better than having a specific DI token to disable server-side rendering (per-route).

Implementing this with a small API surface could be tricky though. Right now the SSR middleware is run before anything else, so we wouldn't see the ctx.type change until the upstream middleware run. It would work, but it means that the render function would still be called, so it's a bit of a waste of compute resources.

@slonoed
Copy link

slonoed commented Mar 1, 2018

it's a bit of a waste of compute resources

I believe this should be measured. SSR, in this case, renders 403. Which typically isn't a heavy page.

@KevinGrandon KevinGrandon self-assigned this Mar 1, 2018
@KevinGrandon
Copy link
Contributor

I believe this should be measured. SSR, in this case, renders 403. Which typically isn't a heavy page.

Sounds good, we can test that. For now we're going to at least unblock people by adding additional extensions to the whitelist, and allowing folks to enhance and extend the default behavior in their apps.

I like setting ctx.type more than either of these approaches, and it's something we should continue to evaluate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants