Unify fastboot serving with ember-cli #356

kratiahuja · 2017-02-15T07:48:37Z

TL;DR: fixes #274 . This will unblock one of the FastBoot 1.0 blockers.

Motivation

Currently FastBoot provides users to serve the base page and the assets via the ember fastboot command. It primarily spins up an express server that is responsible for serving the request with a fastboot replaced base page and serves other assets from disk. However, this is similar to what ember cli provides via ember serve. Since we are not the infrastructure provided by ember-cli, development with ember fastboot has become very error prone for various reasons. The PR attempts to resolve those development concerns.

How it works

ember-cli@2.12.0-beta.1 and above introduced a public API in ember-cli that allows addons to be responsible for serving the base page and/or assets while still be able to consume the other nice features of ember serve. It does so by splitting the task of watching the build and serving assets into different middleware. Infrastructures like FastBoot can then hook a middleware between the watcher and server middleware and be responsible for serving the base page request.

If apps are using ember-cli@2.12.0-beta.1 and above, they will be able to serve fastboot rendered base page using ember serve and continue using other features of ember serve. For apps running ember-cli version below 2.12.0-beta.1, they will not be able to use ember serve with FastBoot.

With the new serving logic, you can use curl with ember serve:
curl 'http://localhost:4200/' -H 'Accept: text/html'

This PR does the following:

Marks ember-cli-fastboot addon to run before broccoli-serve-files addon. broccoli-serve-files is an in-repo addon in ember-cli
Uses serverMiddleware API of ember-cli to provide a middleware to use fastboot-express-middleware when there is a base request.
Added a message when fastboot is serving the base page
Added deprecation message in ember fastboot
For any other requests other than base page, it forwards the request to the next middleware in the chain.
Live reloads the FastBoot sandbox with rebuilt app and vendor files whenever there is a rebuild.
Fixed existing tests
Added new tests that test the same functionality as ember fastboot

Advantages

With using ember serve to have FastBoot serve the initial base page request we get the following features for free:

Ability to show build errors when the initial page request is made so the development is easier and better
Not requiring any infrastructure to explicitly build an express app just for serving.
Ability to have other assets served by ember cli as it does for vanilla app
Ability to use a proxy server for serving any API requests or any other requests using ember serve --proxy=http://localhost:3000
Ability to have live reload in the browser using ember-cli live reload functionality.
Ability to load the built assets from tmp directory instead of dist.

Deprecation of `ember fastboot`

Since this PR does similar to what ember fastboot --watch --serve-assets provides, we will be deprecating ember fastboot command.

Future TODOs

Following are the minor enhancements that I would like to fix on after this PR lands. They do not block this PR.

Make FastBoot live reloading much smarter. We do not need to reload the sandbox if there are only CSS changes. Therefore, we could use fs-tree-diff to see if any of the app/vendor files have changed on rebuild and only load it sandbox then.
Allow developers to provide sandboxGlobals and/or custom sandbox class when serving using FastBoot in development. No one seems to be using this so just added a TODO for now.
Provide a way to turn off fastboot serving at runtime without needing to remove ember-cli-fastboot

cc: @rwjblue @tomdale @danmcclain @stefanpenner @josemarluedke @arjansingh

kratiahuja · 2017-02-21T00:58:30Z

This is now ready for review.

kratiahuja · 2017-02-21T16:20:35Z

Note to self:

Test an app with containing broccoli-asset-rev and make sure it still uses fingerprinted assets in manifest.

stefanpenner · 2017-02-21T17:21:43Z

index.js

+      // TODO: we can do a smarter reload here by running fs-tree-diff on files loaded
+      // in sandbox (explore later).
+      this.ui.writeLine('Reloading FastBoot...');
+      this.fastboot.reload({


Is fastboot reload synchronous? If it is asynchronous, we may need to do more work to ensure no requests during this period are served.

It is synchronous. I confirmed that 😄 Though I plan to make it promise friendly when I take a stab at the smarter reload handling.

We can implement the instrumentation hook (ember-cli@2.13+) and detect the output files that have changed, to see if we need to reload fastboot here.

Agreed. That will eliminate a lot of boiler plate code that I planned to write :)

stefanpenner · 2017-02-21T17:24:02Z

index.js

+            fastboot: this.fastboot
+          });
+
+          fastbootMiddleware(req, resp, next);


the conditional on line 160 suggests we only serve fastbootMiddelware for the first request, subsequent requests will fallback to the else case.

Yes that is correct (I assume you meant line 158 and not 160). If I understand correctly, FastBoot is only meant for initial requests and that too for the base page request (basically have your app be server side rendered for the initial request). Once the base page request is served, the browser will request for assets which should be served by ember-cli's asset serving. Same also applies for subsquent page transitions in Ember (for lazy engines/lazy template loading usecase).

Do you think that is a wrong assumption? Is there a usecase that I might have missed?

Do you think that is a wrong assumption? Is there a usecase that I might have missed?

Ya, that isn't quite correct. If a user hard reloads (or issues curl repeatedly), they should once again get a fastboot rendered page.

@stefanpenner They will get a fastboot rendered page. FastBoot is just responsible for serving the index.html with the rendered content. I did test both use cases on a test app. I am not sure I am following what is not correct.

Your right. for some reason i read line 174 being within the if statement of 161. Not sure how that happened, my bad.

mfazekas · 2017-02-22T11:05:34Z

@kratiahuja with this change ember serve will be serving fastboot version if i point my browser to http://localhost:4200/. Is there a way to ask for regular (non fastboot) version. Other than remove ember-cli-fastboot from package.json? Usecase is that you see some error with fastboot and want to check if it happens in non fastboot version too.

danmcclain · 2017-02-22T12:57:24Z

Just spitballing: what if we could skip injected middleware via query params

rwjblue · 2017-02-21T20:01:38Z

index.js

+          // forward the request to the next middleware (example other assets, proxy etc)
+          next();
+        }
+      }.bind(this))


I think we can use arrow functions instead of .bind(this).

Yeah I totally forgot ember-cli allows arrow functions now. Will update this.

Updated to use arrow functions.

kellyselden · 2017-02-22T15:18:30Z

@mfazekas @danmcclain https://github.com/ember-fastboot/ember-cli-fastboot#double-build-times-and-no-incremental-builds this is how you can run a normal build without removing the ember-cli-fastboot package.

danmcclain · 2017-02-22T16:29:01Z

@kellyselden I was thinking of a way that wouldn't require you to restart the server (and also double build times are going away 🙌 )

kratiahuja · 2017-02-22T19:57:08Z

@mfazekas @danmcclain I agree this is a valid usecase but not scoped to this PR. I want to first get base functionality in and then work on this enhancement and others (see future section). I also like the idea of the query param to switch fastboot serving on/off at run time. I was also thinking of providing it via .embercli config file if possible. Mind opening a separate issue for this and I will be happy to address it once this is merged? I would like to evaluate all options on the table and implement the best feasible one.

stefanpenner · 2017-02-24T21:22:21Z

index.js

+            fastboot: this.fastboot
+          });
+
+          fastbootMiddleware(req, resp, next);


Your right. for some reason i read line 174 being within the if statement of 161. Not sure how that happened, my bad.

kratiahuja · 2017-03-02T03:06:44Z

Tested this PR with fastboot-website app on ember-cli version 2.12.0-beta.1 and 2.11.0 (this is the current one) and both worked as expected with ember fastboot --serve-assets and ember serve commands.

cc: @rwjblue

mfazekas · 2017-03-02T07:22:14Z

We also tested with our app percy-web and it was working great, we needed it for proxy support. (Only issue is FASTBOOT_DISABLED is broken with this PR as it disables fb build but this PR tries to serve it, but that should be addressed in #358).

kratiahuja · 2017-03-03T22:03:50Z

@rwjblue could you please review this? 😄

josemarluedke · 2017-03-04T05:25:52Z

Awesome work @kratiahuja!

I tested this branch in our application as well as in brand new ember app and it works as a charm. 🎉

Looking forward to have this merged and available to users.

mirague · 2017-03-15T16:55:00Z

This looks good!

bcardarella · 2017-03-27T07:10:12Z

Is there anything else blocking this PR?

kratiahuja · 2017-03-27T20:17:14Z

@bcardarella Nope nothing is blocking this PR except someone reviewing it. I'll ping @rwjblue or someone else again after the ember conference.

bcardarella · 2017-03-27T20:24:02Z

Didn't @stefanpenner already approve?

kratiahuja · 2017-03-27T20:28:31Z

@bcardarella Yes but I think @rwjblue also wanted to review before merging. Unfortunately I don't have merge powers so we will have to wait for someone with merge powers to review and merge.

kratiahuja · 2017-04-06T01:11:17Z

Bumping this again. @rwjblue could you please review?

rwjblue · 2017-04-07T13:54:14Z

lib/commands/fastboot-build.js

@@ -20,7 +20,8 @@ module.exports = {
      project: this.project
    });

-    deprecate("Use of ember fastboot:build is deprecated. Please use ember build instead.");
+    deprecate('ember fastboot:build will be deprecated and removed after FastBoot 1.0 is released.');


I'm not sure why we need to add this extra deprecation here. It seems like the preexisting one should already cover the case we care about, no?

Yeah but I just wanted to update the deprecation to say we will be dropping this command too.

rwjblue · 2017-04-07T13:59:30Z

lib/commands/fastboot.js

+      var checker = new VersionChecker(this);
+      var dep = checker.for('ember-cli', 'npm');
+
+      this.ui.writeWarnLine('`ember fastboot` will no longer work after FastBoot 1.0 is released.');


Lets only emit this deprecation when ember-cli is less than 2.12.0-beta.1 (basically the else case of the if just below).

rwjblue · 2017-04-07T14:06:21Z

Only two super minor changes around messaging, then this is good to go...

kratiahuja · 2017-04-07T14:38:38Z

@rwjblue This is good to merge now. Travis is 💚

rwjblue · 2017-04-07T14:40:28Z

Thank you for all your hard work on this @kratiahuja!

simonihmig · 2017-04-07T14:46:46Z

Awesome, thanks indeed! 🎉

This was referenced Feb 15, 2017

unify ember fastboot --watch + ember server #274

Closed

Force ember-cli path to be one of addon when running ember commands. tomdale/ember-cli-addon-tests#64

Merged

kratiahuja changed the title ~~[WIP]: Unify fastboot serving with ember-cli~~ Unify fastboot serving with ember-cli Feb 21, 2017

stefanpenner reviewed Feb 21, 2017

View reviewed changes

stefanpenner requested changes Feb 21, 2017

View reviewed changes

rwjblue reviewed Feb 22, 2017

View reviewed changes

simonihmig mentioned this pull request Feb 22, 2017

The Road to 1.0 #98

Closed

38 tasks

stefanpenner approved these changes Feb 24, 2017

View reviewed changes

kratiahuja mentioned this pull request Feb 24, 2017

Expose request.method and request.body in Fastboot service #354

Closed

mfazekas mentioned this pull request Feb 25, 2017

disable fastboot with unified serving #358

Closed

stefanpenner approved these changes Mar 3, 2017

View reviewed changes

kratiahuja mentioned this pull request Mar 7, 2017

Race condition prevents cleaning of dist folder between rebuilds #246

Closed

rwjblue reviewed Apr 7, 2017

View reviewed changes

Fastboot serve refactor (unifying the experience with ember-cli)

ecdb661

rwjblue approved these changes Apr 7, 2017

View reviewed changes

rwjblue merged commit 38892b1 into ember-fastboot:master Apr 7, 2017

kratiahuja deleted the serve-refactor branch April 7, 2017 14:40

This was referenced May 21, 2017

ember build performance regression #159

Closed

Target Support in RFC #268

Closed

Unify fastboot serving with ember-cli #356

Unify fastboot serving with ember-cli #356

Conversation

kratiahuja commented Feb 15, 2017 • edited Loading

Motivation

How it works

Advantages

Deprecation of ember fastboot

Future TODOs

kratiahuja commented Feb 21, 2017

kratiahuja commented Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

kratiahuja Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

rwjblue Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratiahuja Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

stefanpenner Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfazekas commented Feb 22, 2017 • edited Loading

danmcclain commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyselden commented Feb 22, 2017

danmcclain commented Feb 22, 2017

kratiahuja commented Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

kratiahuja commented Mar 2, 2017 • edited Loading

mfazekas commented Mar 2, 2017

kratiahuja commented Mar 3, 2017

josemarluedke commented Mar 4, 2017 • edited Loading

mirague commented Mar 15, 2017

bcardarella commented Mar 27, 2017

kratiahuja commented Mar 27, 2017

bcardarella commented Mar 27, 2017

kratiahuja commented Mar 27, 2017

kratiahuja commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Apr 7, 2017

kratiahuja commented Apr 7, 2017

rwjblue commented Apr 7, 2017

simonihmig commented Apr 7, 2017

kratiahuja commented Feb 15, 2017 •

edited

Loading

Deprecation of `ember fastboot`

kratiahuja commented Feb 21, 2017 •

edited

Loading

kratiahuja Feb 21, 2017 •

edited

Loading

rwjblue Feb 21, 2017 •

edited

Loading

kratiahuja Feb 21, 2017 •

edited

Loading

stefanpenner Feb 24, 2017 •

edited

Loading

mfazekas commented Feb 22, 2017 •

edited

Loading

kratiahuja commented Feb 22, 2017 •

edited

Loading

kratiahuja commented Mar 2, 2017 •

edited

Loading

josemarluedke commented Mar 4, 2017 •

edited

Loading