Proposal: Implement additional SSR detection and control

[KevinGrandon]

This implements two SSR detection features.

• Additional non-SSR whitelisted extensions
  Adds the following extensions: (js|gif|jpg|png|pdf|json)

• SSRDeciderToken for enhancing SSR logic
  Exports our current SSR logic as a new token, SSRDeciderToken, which can be enhanced through the DI system. This is a change which keeps our API surface fairly small, isn't difficult to support, and allows folks to control server rendering while we iterate on a future solution.

For more discussion and a breakdown of supported URIs see: #116 (comment)

Fixes #45
Fixes #116

[codecov]

Codecov Report

Merging #176 into master will increase coverage by 0.37%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   93.39%   93.76%   +0.37%     
==========================================
  Files          16       16              
  Lines         333      337       +4     
  Branches       65       65              
==========================================
+ Hits          311      316       +5     
  Misses         13       13              
+ Partials        9        8       -1

Impacted Files	Coverage Δ
src/index.js	100% <ø> (ø)	⬆️
src/base-app.js	95.19% <100%> (+0.04%)	⬆️
src/tokens.js	100% <100%> (ø)	⬆️
src/server-app.js	100% <100%> (ø)	⬆️
src/plugins/ssr.js	95.65% <88.88%> (+1.62%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6f7bb7...ea852bd. Read the comment docs.

[rajeshsegu]

does it help to also test for method === 'GET'

[ganemone]

yea we should add that check

[KevinGrandon]

Sounds good 👍 Let's do this in another PR though so the code + tests don't distract from the core issue in this proposal.

[lhorie]

Just yesterday, I was writing an analysis of various methods that had been proposed here: #116 (comment)

This implements one of the recommendations. @slonoed's use case can potentially be better supported out-of-box via the other recommendation

With regards to this proposal, I think the injected service should be a function that can early return before the base algorithm. Replacing it altogether via enhance requires adding a lot of composition boilerplate for the most common case, and if said boilerplate is not implemented, you lose default semantics and might potentially break unrelated scenarios.

i.e.

// do something like
app.register(SSRBlacklistToken, () => ['/path/with/no/ssr'])

// instead of
const SSRDeciderEnhancer = ssrDecider =>
  createPlugin({
    provides: () => ctx => {
      return ssrDecider(ctx) && !ctx.path.match(/paths|with|no|ssr/);
    },
  });
app.enhance(SSRDeciderToken, SSRDeciderEnhancer);

[KevinGrandon]

I did not see #116, but that is a very nice analysis. I think that this falls under the Provide a isSSR DI hook for power users, which can potentially allow people to completely override the existing SSR logic if necessary. I like the enhance more than an early return fn because it's more powerful.

I think that if we couple this, along side extension checking in the default SSRDecider plugin, that will give us the best of both worlds. A basic extension check for most cases, and also enhancing for power users.

(I'm happy to add extension checking to this PR, or we can do it in another PR)

[lhorie]

I think that this falls under the Provide a isSSR DI hook for power users

Yeah, interesting that we came up with similar solutions independently :)

I like the enhance more than an early return fn because it's more powerful.

Hmm, that's a good point.

Something that just occurred to me: why do we need to return a plugin? Can we do:

const SSRDecider = decide =>
  ctx => decide(ctx) && !ctx.path.match(/paths|with|no|ssr/);

Also, can we simplify the base check?

const SSRDecider = isSSRSuggested =>
  ctx => isSSRSuggested && !ctx.path.match(/paths|with|no|ssr/);

// or

const SSRDecider = (ctx, isSSRSuggested) =>
  isSSRSuggested && !ctx.path.match(/paths|with|no|ssr/);

[KevinGrandon]

Something that just occurred to me: why do we need to return a plugin? Can we do:

Yes, I think I just like the idea that everything is a plugin, but we should be able to simplify it. We may also want to support the plugin case for deps though, but documenting the simple case is better for users. I will try to make those updates, along with test cases.

[KevinGrandon]

New updates, simplified documented power-user enhancer. The new syntax is pretty clean:

app.enhance(SSRDeciderToken, decide => ctx =>
  decide(ctx) && !ctx.path.match(/ignore-ssr-route/)
);

Also added additional extensions which should solve the majority of cases.

[lhorie]

LGTM

[KevinGrandon]

!merge