`isSSR` should not render as template if the request implies a non-html MIME type #116

lhorie · 2018-01-31T21:14:09Z

Currently, there's no good way of rendering a dynamic PDF other than doing horrible hacks.

We already do the right thing for the .js extension:

fusion-core/src/plugins/ssr.js

Line 61 in fd40195

if (ctx.path.match(/\.js$/)) return false;

This logic should be extended to other MIME types, such as PDF, images, fonts, etc

The text was updated successfully, but these errors were encountered:

lhorie · 2018-01-31T21:19:47Z

Related: #45, #46

mlmorg · 2018-02-23T23:32:53Z

Need to come up with potential solutions.

lhorie · 2018-03-01T00:21:22Z

Permutations that work today

pros: it's what we have today
cons: doesn't work for asset downloads, and does not address debugging devexp issues

Covered cases: 8/16

Proposed solutions so far and what scenarios they fix

extension regex check

ctx.path.match(/\.(js|gif|jpg|png|pdf|html|json)$/)

pros: covers all classes of use cases (ssr, dynamic asset, static asset, endpoint)
cons: does not address debugging devexp issues

Covered cases: 12/16

extension convention (no-extension = ssr, extension = non-ssr) in addition to accepts header heuristics

pros: same as extension regex check
cons: same as extension regex check

Covered cases: 12/16

asset manifest check

pros: addresses any type of static assets
cons: does not address asset downloads and does not address debugging devexp issues

Covered cases: 10/16

unset ctx.element and ctx.template if ctx.body is set

pros: addresses all cases
cons: brittle (no guarantee it won't throw errors on arbitrary plugin order), hacky (least bad hook requires monkeypatching the koa getter of ctx.body), inneficient (it's still possible to run SSR code before ctx.body is set due to plugin ordering)

Covered cases: 16/16 but poorly

Other concerns

It may also be desirable to show user-friendly 404s for a non-existing route, regardless of whether it would have been ssr'ed via other heuristics.

This requires the ability to arbitrarily override the isSSR check, and could conceivably be supported via DI

app.register(IsSSRToken, ctx => !ctx.path.match(/paths|with|no|ssr/))

pros: addresses all cases
cons: requires manual intervention, potential duplication of routing logic

Covered cases: 16/16

Analysis

Developer patterns (curl, opening an endpoint from the browser) are difficult to support
Asset manifest is better than current strategy but does not handle dynamic endpoints
Extension-based heuristics provide the largest amount of coverage without hacks/caveats. Unsupported cases often align with use cases that have no value.
Heuristics based on the koa context state machine are likely to break in difficult to understand ways

Recommendation

Use an extension-based heuristic (either a blacklist of extensions where ssr should never occur, or generically avoid ssr in any url w/ an extension) to unblock users that need dynamic endpoints and provide a nicer experience out of the box.
Provide a isSSR DI hook for power users
Iterate on potential solutions to the difficult-to-support cases later if the two steps above are not sufficient.

rtsao · 2018-03-01T01:45:09Z

There's something I find really appealing about a solution using a path-prefix, (e.g. a route is SSR if and only if it does not start with _). IMHO, if you are not serving something to be viewed in a browser, then you should not care that much about the URL. Path prefix is dead-simple heuristic and I just don't see the constraint being an issue in any practical case.

I'm less enthused about an extension-based convention, since there's more ambiguity in the case of .html/.htm extensions, which people often expect to be equivalent to the extensionless path for web pages. Add in the common magic regarding rewrite rules in frontends and it comes even more confusing.

slonoed · 2018-03-01T09:07:32Z

@rtsao I think prefix doesn't conform web ideology. URL doesn't imply type, headers do. It also means that routes with _ prefix are forbidden and application which want to use generated routes have to escape this symbol.

Example: I have an application which is more or less file browser (think about s3 file browser). And I want to show file path as URL path, like https://example.com/any/name/path. A path https://example.com/_any/name/path is broken now. To avoid this I have to replace symbol (which is super weird) or create prefix path https://example.com/files/_any/name/path.

Take a look at this comment #45 (comment), maybe it could be a better way.

KevinGrandon · 2018-03-01T20:32:49Z

We have implemented both parts 1) and 2) of your suggestion. I think we'll still need to iterate on this to make it nicer for entries in the asset manifest, and also potentially per-plugin setting of headers. People should generally be unblocked now, and we can continue making this better over time.

lhorie added the bug label Jan 31, 2018

lhorie mentioned this issue Jan 31, 2018

Update check in isSSR function to use asset manifest #46

Open

lhorie mentioned this issue Mar 1, 2018

Proposal: Implement additional SSR detection and control #176

Merged

KevinGrandon self-assigned this Mar 1, 2018

old-fusion-bot bot closed this as completed in #176 Mar 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`isSSR` should not render as template if the request implies a non-html MIME type #116

`isSSR` should not render as template if the request implies a non-html MIME type #116

lhorie commented Jan 31, 2018 •

edited

lhorie commented Jan 31, 2018 •

edited

mlmorg commented Feb 23, 2018

lhorie commented Mar 1, 2018 •

edited

rtsao commented Mar 1, 2018 •

edited

slonoed commented Mar 1, 2018

KevinGrandon commented Mar 1, 2018

isSSR should not render as template if the request implies a non-html MIME type #116

isSSR should not render as template if the request implies a non-html MIME type #116

Comments

lhorie commented Jan 31, 2018 • edited

lhorie commented Jan 31, 2018 • edited

mlmorg commented Feb 23, 2018

lhorie commented Mar 1, 2018 • edited

Permutations that work today

Proposed solutions so far and what scenarios they fix

extension regex check

extension convention (no-extension = ssr, extension = non-ssr) in addition to accepts header heuristics

asset manifest check

unset ctx.element and ctx.template if ctx.body is set

Other concerns

Analysis

Recommendation

rtsao commented Mar 1, 2018 • edited

slonoed commented Mar 1, 2018

KevinGrandon commented Mar 1, 2018

`isSSR` should not render as template if the request implies a non-html MIME type #116

`isSSR` should not render as template if the request implies a non-html MIME type #116

lhorie commented Jan 31, 2018 •

edited

lhorie commented Jan 31, 2018 •

edited

lhorie commented Mar 1, 2018 •

edited

rtsao commented Mar 1, 2018 •

edited