ember-fastboot / ember-cli-fastboot

Server-side rendering for Ember.js apps
http://ember-fastboot.com/
MIT License
852 stars 160 forks source link

FastBoot still renders the body on redirect #802

Open SergeAstapov opened 3 years ago

SergeAstapov commented 3 years ago

Looks like it's a miss in the implementation as outlined in https://github.com/ember-fastboot/ember-cli-fastboot/pull/195#issue-70081165:

transitionTo still renders the body. The middleware or fastboot could be modified to not include it, but a better solution would be to tell Ember to abort the page rendering. Tips on how to do that would be appreciated. I couldn't find anything straightforward in the docs.

fastboot library does not include body for redirects, implemented in commit https://github.com/ember-fastboot/fastboot/commit/3f386fd82cc2d8733d080d3204420223ecc57666.

Since then lot of things have changed in Ember rendering engine and I hope this is feasible/possible to implement to tell Ember not to render anything.

Looks like shouldRender configuration option would result in desired outcome but it's set too early in bootstrap phase.

Any other ideas?

kiwiupover commented 3 years ago

Hello @SergeAstapov can you provide a recreation of this issue.

I would be happy to jump on a call to understand your issues.

SergeAstapov commented 3 years ago

Hi @kiwiupover! Sorry for delay, link to simple reproduction is https://github.com/SergeAstapov/fastboot-repro-802

Actual steps to minimal reproduction are simple, here is the commit which adds two routes where A route does redirect to B route.

I added console.log() statement to model hook for routes A and B and by navigating to route A via curl you can see route B actually get invoked (console.log() from model hook runs) + template content gets rendered.

Screen Shot 2020-12-06 at 11 14 49 PM

The rendered html from Ember gets ignored and simple redirect message returned by Result here.

Ideally, I would expect Ember not to follow redirects in FastBoot mode and do not render any DOM in FastBoot mode for redirects as it's simply ignored and not used for the response.

SergeAstapov commented 3 years ago

Hi @kiwiupover! Given the above explanation and reproduction, do you have enough information on the issue? Happy to provide more info if needed.

For the future readers should they end up in the same situation:

Currently we end up setting a flag on ApplicationController from the redirect hook of the respective route route like so

export default class RouteA extends Route {
  ...
  redirect (model) {
    if (/* some redirect condition based on model */ && this.fastboot.isFastBoot) {
      getOwner(this).lookup('controller:application').shouldRender = false;
      this.transitionTo('route-b');
    }
  }
}

given ApplicationController has default value for shouldRender:

# controllers/application.js
export default class ApplicationController extends Controller {
  shouldRender = true;
}

and then in application.hbs we wrap all the content into condition

{{#if this.shouldRender}}
  ...
{{/if}}
hoIIer commented 2 years ago

@SergeAstapov @kiwiupover just hit this same error when adding a redirect for a base route... and it exposes my prod s3 bucket when fastboot returns the redirect body from above!

Ideally I'd like to not have to wrap my application in a flag, is there a settled way to handle this in 2021?

router.js

  ...
  this.route('legal', function() {
    this.route('tos');
  });
import Route from '@ember/routing/route';

export default class LegalIndexRoute extends Route {
  redirect() {
    this.transitionTo('legal.tos');
  }
}

going to site.com/legal will render the redirect body instead of the actual ember redirected page.

edit: after looking at the source code, found a way around it.

https://github.com/ember-fastboot/ember-cli-fastboot/blob/2a6ed5d76b00da2106a9ba10f048aa2bef989a80/packages/fastboot/src/result.js#L48

for redirects visit().then((response) => response.html()) hits L40

so essentially you have to do something like this

          // render the page with fastboot.
          const res = await this.fastboot.visit(
            this.request.url,
            {request: this.request},
          );

          if (res.statusCode >= 300 && res.statusCode <= 399) {
            html = await res.domContents().body;
          } else {
            html = await html.html();
          }

not sure how common it is to render fastboot page manually like this but perhaps an optional arg to html() to render redirects could be helpful

Additionally this is in the context of rendering a fastboot app inside a cloud cdn and not through a node server.

edit 2

the above didn't work as I naively didn't realize I was only getting the html body from domContents().body (duh)

so far the only thing I can think of is a monkey patch/hack

          const res = await this.fastboot.visit(
            this.request.url,
            {request: this.request},
          );

          if (res.statusCode >= 300 && res.statusCode <= 399) {
            // trick fastboot into thinking it was a successful request.
            res._fastbootInfo.response.statusCode = 200;
          }

          // render the pages html.
          html = await res.html();

could open a pr