Refactor fastboot build to be more performant by kratiahuja · Pull Request #369 · ember-fastboot/ember-cli-fastboot

kratiahuja · 2017-04-02T02:11:43Z

TL;DR: This unblocks FastBoot 1.0 release with a breaking change. Resolves most of the issues mentioned in #360, #292, #264, #246 .

Sending out a PR since substantial work is done that it can be reviewed for directional purposes.

NOTE: I took a slightly different approach in how the fastboot initializers will be written per the spec

Fixes in this PR currently are

TODO (In subsquent PRs):

Unskip tests using ember-cli-head once that addon embraces new format.
Remove TODOs from index.js
README updates
Doc updates
Kill ember fastboot command 🔪
Update some of the exisiting addons to not rely on process.env.EMBER_CLI_FASTBOOT flag or not contain app/[instance-]initializers/[browser|fastboot]/*.js folder structure (owner: @simonihmig since he has already started this work)

cc: @rwjblue @tomdale @danmcclain @ryanone @simonihmig @arjansingh @stefanpenner

kratiahuja · 2017-04-02T02:12:12Z

Please feel free to try this out in your app without any other fastboot complaint addons.

kratiahuja · 2017-04-15T20:39:07Z

#374 resolves Expose a hook to add to manifest files TODO. Created a separate PR since it will be needed before this PR can land. Once it is merged, will require some minor refactoring in this PR since buildFastbootTree is no longer present.

simonihmig · 2017-04-24T14:40:38Z

lib/build-utilities/migrate-initializers.js

+
+  if (isBrowserInitializersPresent) {
+    var errorMsg = `FastBoot build no longer supports ${rootPath}/app/(instance-)?initializers/browser structure. ` +
+                         `Please refer to www.ember-fastboot.com for a migration path.`;


@kratiahuja I have picked up migrating an addon as we have discussed on Slack, and after updating ember-cli-fastboot to the latest version of your PR, I immediately ran into this error message, being caused by some other (unknown) addon.

The problem I see here is that a) probably every existing app will run into this error once trying to migrate to FastBoot 1.0, and b) it gives no clue where that offending initializer actually comes from, which could be quite frustrating. It could be even some nested addon dependency the user does not see in his package.json.

I think it would be cool (in this PR or a follow up) to have some better tracing available, like which addon is causing this, or at least what the initializers actual name is, to be able to effectively investigate into this!

Please forget it! ;) I was not paying enough attention to see it actually does exactly this. 🙄
Need some more sleep probably...

Yeah we can't auto-migrate browser based initializers, hence we will need to fail the build. I am happy to refactor the error messages to be more better. Probably sometime this week i'll come up with a migration cheat sheet so that it is easier for addons 😄

That would be awesome! Would be nice to have a link in the above mentioned error message, maybe a dedicated migration page on www.ember-fastboot.com?

Sorry I missed replying to this. Yes would be nice to have a dedicated migration page. I think before this PR is merged, we should send in a warning (but getting another PR) and link to the migration page.

simonihmig · 2017-04-26T10:34:19Z

lib/build-utilities/migrate-initializers.js

+ *
+ * @param {Object} project
+ */
+function migrateInitializers(project) {


I just noticed that this works on the original files. So in my app after running ember s the files were modifed, the old instance-initializer/fastboot/foo.js was physically moved to fastboot/instance-initializer/foo.js (from POV of git a new, not yet added file). The same for addons, some yarn linked addons got silently modified (which I did not want as that would have broken them for pre 1.0 FastBoot).

I wonder if that migration magic should not happen inside some broccoli tree rather than operating on the original files of the app and in node_modules/someAddon?

Why will it break for pre fastboot 1.0? This migration will not apply for pre fastboot 1.0 but only after.

Reason for moving is because if we don't, these initializers will end up running in browser too. Hence it needs to be moved. I am hoping reading the message, addon owners would move the fastboot intiializers themselves post fastboot 1.0. This migration would be dropped after a few releases of fastboot 1.0

The breaking pre FastBoot 1.0 thing was related to the changes in ember-useragent, that you also reviewed. There we left the instance-initializer/fastboot/ember-useragent in place, so it works in pre 1.0 as well as in post 1.0 (because of the automatic migration). But because it was symlinked, and the migration works on the original files, it modified the original addon code, moving the initializer to a the new place...

So this is the point I was trying to make, that this migration did not only move files in a temporary state while building the app like a broccoli plugin would do (take the previous approach of fastboot-filter-initializers as an example, which filters one of the browser|fastboot initializers in a broccoli tree), but it modified the original in-repo files of the app, as well as any symlinked addon (in fact any addon in node_modules). I would expect something like a codemod to do that, but certainly not ember s.

I am like the idea of a codemod doing something like this. But what if they forget to run the codemod tool post 1.0 before serving? It will break for them more. I guess we can detect if they ran codemod too and not serve.

I am actually not arguing in favor of a codemod, I am just arguing that ember s should not modify any original file in my app/addons. It can move things around in a broccoli tree when building, but not the original in-repo files IMHO.

@simonihmig My earlier comment was me just musing on the idea.

Wouldn't moving in broccoli-tree be very expensive? Are you talking about after the treeForApp is generated? I don't think we will solve the build performance problem then (at least in a temporary sense).

I totally agree it is not the best way and I am happy to explore new ways to do it. Do you have an example that you can explain with?

Reason for changing the original in-repo files is (maybe moving in broccoli tree solves the below issue):
the problem not moving in-repo files is (as I mentioned earlier) that if we don't move you will end up with two AMD modules after the build:

<app-name>/initializers/fastboot/x.js <-- This one will get loaded in browser and FastBoot

<app-name>/initializers/x.js <--- this one is only loaded in FastBoot

When the app will boot in the browser, it will run app/initializers/fastboot/x.js (this because we no longer filter the fastboot initializers) which will cause issues. Hence there is a need to move the in-repo files so that we don't generate <app-name>/initializers/fastboot/x.js.

Maybe there is a better way to solve this problem that I am not seeing?

simonihmig · 2017-04-26T10:35:48Z

lib/build-utilities/migrate-initializers.js

+const fs = require('fs-extra');
+
+const fastbootInitializerTypes = [ 'initializers', 'instance-initializers'];
+const FASTBOOT_DIR = 'fastboot';


Given the description of the PR I was expecting the new FastBoot folder to be named fastboot-app rather than fastboot, but this might be just some misunderstanding?

I prefer calling it fastboot instead of fastboot-app since it is more generic

kratiahuja · 2017-04-27T02:18:08Z

Draft and WIP cheatsheet on how fastboot compatible addons can continue to be backward compatible (with this build changes) if they do these changes: https://gist.github.com/kratiahuja/d22de0fb1660cf0ef58f07a6bcbf1a1c

This may change subject to the feedback on this PR and feedback on the cheatsheet. I'll try bringing this up in the next FastBoot meeting.

cc: @simonihmig

kratiahuja · 2017-04-27T18:52:59Z

Removing WIP since this is ready to review. The review has got pretty large that we can do the remaining TODOs in a subsquent PR.

simonihmig · 2017-04-27T21:13:10Z

index.js

+
+      // invoke addToFastBootTree for every addon
+      if (addon.addToFastBootTree) {
+        var additionalFastBootTree = addon.addToFastBootTree();


Related to this problem: ember-fastboot/ember-cli-head#21 (comment)
Is it actually required that addons return a tree that was created by lib/build-utilities/fastboot-builder.js? Isn't it possible that the addon just return a simple tree (like this.treeGenerator(path)), and the mergedFastBootTree (Line 152) is then moved to appName (Funnel destDir)?

I think we could do that that. Will need to try it. This would also enable linting automatically if I remember correctly (cc: @tsubomii).

how does this differ from treeForFastboot and would the following cover this use-case?

treeForFastboot(tree) { return mergeTrees([ tree, // `<addon-root>/fastboot/` additionalTree // from whereve you would want. ]); }

note: at first glance ^ would feel more aligned to existing API's, but let me know if I am missing someting

@simonihmig Fixed. You can just use this.treeGenerator(path) and return that.

@stefanpenner I actually like the idea of having treeForFastBoot which is more aligned to other existing APIs. And yes it would cover the usecase I wanted it to cover as well. Will update it to providing treeForFastBoot :)

stefanpenner · 2017-05-05T20:30:28Z

index.js

    this._super.init && this._super.init.apply(this, arguments);
+    // set a environment variable to allow addons to use `fastboot-filter-initializers`
+    // for old versions.
+    process.env.FASTBOOT_NEW_BUILD = true;


would it make more sense to set this when this module is included rather than later when it is initialized? That may reduce the window where this variable appears to be unset but will soon be set.

I wanted this variable to be set early on. Isn't included called after init?

stefanpenner · 2017-05-05T20:34:15Z

index.js

+
+      // invoke addToFastBootTree for every addon
+      if (addon.addToFastBootTree) {
+        var additionalFastBootTree = addon.addToFastBootTree();


how does this differ from treeForFastboot and would the following cover this use-case?

treeForFastboot(tree) { return mergeTrees([ tree, // `<addon-root>/fastboot/` additionalTree // from whereve you would want. ]); }

note: at first glance ^ would feel more aligned to existing API's, but let me know if I am missing someting

stefanpenner · 2017-05-05T20:36:14Z

index.js

+    });
+
+    var fileAppName = path.basename(this.app.options.outputPaths.app.js).split('.')[0];
+    var finalFastbootTree = Concat(processExtraTree, {


new Concat

stefanpenner · 2017-05-05T20:36:55Z

index.js

+    }
+    trees.push(fastbootTree);
+
+    let newTree = new mergeTrees(trees);


new MergeTrees or no new if lowercase.

stefanpenner · 2017-05-05T20:37:31Z

index.js

+    var fastbootConfig = config.fastboot;
+    // do not boot the app automatically in fastboot. The instance is booted and
+    // lives for the lifetime of the request.
+    if (config.hasOwnProperty('APP')) {


'APP' in config

stefanpenner · 2017-05-05T20:42:31Z

lib/build-utilities/migrate-initializers.js

+
+    const fastbootDirPath = path.join(rootPath, FASTBOOT_DIR);
+    // check if fastboot/app exists
+    if(!existsSync(fastbootDirPath)) {


missing space between if and (

stefanpenner · 2017-05-05T20:43:06Z

lib/build-utilities/migrate-initializers.js

+
+    // copy over app/initializers/fastboot and app/instance/initializers/fastboot
+    fastbootInitializerTypes.forEach((fastbootInitializerType) => {
+      var srcFastbootPath = path.join(rootPath, 'app', fastbootInitializerType, 'fastboot');


if we are using const and => could we also use let in-place of var?

Yup definitely yes :)

stefanpenner · 2017-05-05T20:45:28Z

package.json

  "dependencies": {
-    "broccoli-funnel": "^1.0.0",
+    "broccoli-concat": "^3.2.2",
+    "broccoli-funnel": "^1.1.0",


v1.2.0 is latest

stefanpenner · 2017-05-05T20:46:19Z

package.json

    "ember-cli-babel": "^5.1.7",
    "ember-cli-eslint": "^3.0.2",
    "ember-cli-version-checker": "^1.3.1",
+    "exists-sync": "0.0.4",


I think we should just use fs.existsSync as it is provided by the STDLIB, although fs.exists is deprecated the sync variant is not.

I thought fs.existsSync is deprecated. The readme of exists-sync says this: https://github.com/ember-cli/exists-sync#exists-sync

fs.existsSync was deprecated along with fs.exists a long way back in nodejs/node@7b5ffa4, but then fs.existsSync was undeprecated in nodejs/node@7b5ffa4.

Aah didn't know that. Will use fs.existsSync and update readme for exists-sync too.

stefanpenner · 2017-05-05T20:47:08Z

test/head-content-test.js


-describe('head content acceptance', function() {
+// TODO: Unskip this test once ember-cli-head is released with new fastboot build spec.
+describe.skip('head content acceptance', function() {


ember-fastboot/ember-cli-head#21 right?

stefanpenner · 2017-05-05T21:11:54Z

Left some thoughts, going to test it out manually next :)

kratiahuja · 2017-05-05T21:14:09Z

@stefanpenner Thank you so much for taking time to review a very very long PR :) Really appreciate it. I'll address the comments this weekend!

stefanpenner · 2017-05-05T21:19:38Z

Thank you so much for taking time to review a very very long PR

NP! Excited to see progress on this front!

I'll address the comments this weekend!

Some of them are intended more as discussion (don't assume I'm right)

kratiahuja · 2017-05-05T21:20:28Z

Some of them are intended more as discussion (don't assume I'm right)

Yup I meant I'll reply back this weekend :)

stefanpenner · 2017-05-05T21:44:21Z

Local testing is looking good, rebuild times feel great. Everyone seems to work.

I was a tad surprised that curl 'http://localhost:4200/' didn't include the fast boot content but
curl 'http://localhost:4200/' -H 'Accept: text/html' did. But I guess this resolves some ambiguity and mimics the browser.

stefanpenner

LGTM, IMHO lets

@tomdale c/d?

mazondo · 2017-05-15T22:08:00Z

Anything I can help with to push this over the edge - we're keen on using it in our fastboot migration

stefanpenner · 2017-05-15T22:39:49Z

Anything I can help with to push this over the edge - we're keen on using it in our fastboot migration

We have a meeting tomorrow afternoon/evening to get this shipped. If delegatable tasks fall out, we will open them as issues :)

Best you can do to help us today, is test out this PR and report issues.

scottmessinger · 2017-05-16T18:48:02Z

I tried out this branch and it's working great! The one problem I've encountered is handling app.import. The discussion in this issue seems to focus on addons. In my app, addons aren't really the problem -- vendor libraries are. I'm importing libraries like interact.js, CKEditor, iscroll, etc and I had to go into each file in /vendor and add if (window.document !== undefined) {...}. Upgrading the libraries in the future will be harder since I'll have to copy and paste the library into the if block.

In addition to converting my vendor libs over, I tried getting ember-fetch working with this branch using the solution proposed here https://gist.github.com/kratiahuja/d22de0fb1660cf0ef58f07a6bcbf1a1c and working from the PR here: https://github.com/tomdale/ember-network/pull/20/files. I found the instructions in the gist to be somewhat confusing. I really appreciate @kratiahuja for drafting that up and I acknowledge that some of the complexity might be necessary. I'm hoping it can avoided and tucked behind a more declarative API.

Before this PR, the if (isFastBoot){...} else {....} was clear because it was a simple branch -- do the opposite if Fastboot is there. Now, there's a wildly different way to include a lib for Fastboot vs not including it. For instance, when including for the browser, a file can be included without being evaled if it's filtered out in treeForVendor. A couple steps, but they're logical. However, the opposite -- only include a file for fastboot is a completely different technique, requiring knowledge of the fastboot manifest.

In working with ember-fetch and my own app, what I wanted was a much simpler, more declarative way of filtering libs for Fastboot: app.import('libname.js', {loadInFastBoot: false}). I'm not sure if an API like that is possible, but I think it would be a helpful improvement.

kratiahuja · 2017-05-16T20:50:14Z

Thanks @scottmessinger for your feedback. I appreciate it.

I had to go into each file in /vendor and add if (window.document !== undefined) {...}. Upgrading the libraries in the future will be harder since I'll have to copy and paste the library into the if block.

You could do this using broccoli-stew as well to avoid copy and paste issues in future. You could use an in-repo addon too for such a purpose.

In working with ember-fetch and my own app, what I wanted was a much simpler, more declarative way of filtering libs for Fastboot: app.import('libname.js', {loadInFastBoot: false}). I'm not sure if an API like that is possible, but I think it would be a helpful improvement.

I agree with your thoughts on this. Did you read the build spec here ? There is an unresolved item about app.import that we can address in future. The goal of this PR is to come up with a minimal set of changes required to make the build more performant and make it to cut FastBoot major release version.

Before this PR, the if (isFastBoot){...} else {....} was clear because it was a simple branch -- do the opposite if Fastboot is there. Now, there's a wildly different way to include a lib for Fastboot vs not including it. For instance, when including for the browser, a file can be included without being evaled if it's filtered out in treeForVendor. A couple steps, but they're logical. However, the opposite -- only include a file for fastboot is a completely different technique, requiring knowledge of the fastboot manifest.

I understand that but the new build schema is different than the current one. The idea is instead of forking assets for different environments you want to load the browser assets and additional fastboot assets in fastboot environment. So you will need to load the fastboot assets in a different way. An app that is being server side rendered will nneed to have knowledge of fastboot manifest. I don't think that is wrong assumption.

I found the instructions in the gist to be somewhat confusing. I really appreciate @kratiahuja for drafting that up and I acknowledge that some of the complexity might be necessary. I'm hoping it can avoided and tucked behind a more declarative API.

The gist is very much in early draft stages. It was meant to iron out the different usecases where we need a migration path forward. As mentioned in #360 there needs to be a migration guide to help addon owners.

scottmessinger · 2017-05-17T13:44:59Z

@kratiahuja Thanks for the response! I'm excited you're open to letting app.import filter fastboot assets.

An app that is being server side rendered will need to have knowledge of fastboot manifest. I don't think that is wrong assumption.

While I do think it's completely reasonable an app/addon to have knowledge of fastboot, I don't follow why an app/addon needs an understanding of the fastboot manifest. When "you want to load the browser assets and additional fastboot assets in fastboot environment", I think you'd only need a higher level api (like app.import('fileName', {loadInFastboot: false})). While having lower level primitives (involving the fastboot manifest) is a sound idea, in my mind, it's better to keep those primitives private in order to reduce the surface area of the public API one needs to learn just to get their app/addon working in fastboot. That all said, you know way, way more about fastboot than I, so maybe requiring addon/app developers to learn the fastboot manfest and more Broccoli callbacks is important and necessary.

Totally understand about the gist being an early draft -- I figured as much and really appreciate that you wrote it up.

stefanpenner · 2017-05-17T14:53:15Z

app.import('fileName', {loadInFastboot: false})).

I believe this already exists, or at-least the machinery to make it work.

app.import(thing, { outputFile: 'fastboot/some-file-only-required-in-input.js' }

But this discussion should likely find a new issue / rfc or something.

stefanpenner · 2017-05-17T16:34:45Z

Awesome job @kratiahuja!

stefanpenner · 2017-05-17T16:36:37Z

We should cut a beta today, then ask for community help in upgrading existing add-ons.

Burn down issue for addons that need <3 #387

kratiahuja mentioned this pull request Apr 2, 2017

Master punch list for resolving Fastboot 1.0 build issues #360

Closed

35 tasks

simonihmig reviewed Apr 24, 2017

View reviewed changes

This was referenced Apr 24, 2017

Fix broken FastBoot build, add more FastBoot tests ember-animation/liquid-fire#570

Merged

Prepare for FastBoot 1.0 breaking changes willviles/ember-useragent#1

Merged

simonihmig reviewed Apr 26, 2017

View reviewed changes

kratiahuja mentioned this pull request Apr 26, 2017

Prepare for FastBoot 1.0 ember-fastboot/ember-cli-head#21

Merged

kratiahuja changed the title ~~[WIP]: Refactor fastboot build to be more performant~~ Refactor fastboot build to be more performant Apr 27, 2017

simonihmig reviewed Apr 27, 2017

View reviewed changes

stefanpenner reviewed May 5, 2017

View reviewed changes

stefanpenner mentioned this pull request May 8, 2017

CSS hot reloading not working #293

Closed

Build WIP

b4c105c

stefanpenner mentioned this pull request May 12, 2017

ember fastboot does not terminate with ember-cli >= 2.13 #386

Closed

stefanpenner approved these changes May 12, 2017

View reviewed changes

stefanpenner merged commit 8cb1542 into ember-fastboot:master May 17, 2017

kratiahuja mentioned this pull request May 18, 2017

Unskip ember-cli-head usage test #393

Closed

jasonmit mentioned this pull request May 21, 2017

Rewrite fastboot implementation empress/ember-cli-showdown#51

Closed

buschtoens mentioned this pull request May 22, 2017

Doesn't work with ember-cli-fastboot@1.0.0-rc.1 ember-cli/ember-page-title#69

Closed

bobisjan mentioned this pull request Jan 6, 2018

Fastboot Safe san650/ember-cli-page-object#348

Merged

Comments

Conversation

kratiahuja commented Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes in this PR currently are

TODO (In subsquent PRs):

Uh oh!

kratiahuja commented Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kratiahuja commented Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kratiahuja Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonihmig Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kratiahuja Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kratiahuja commented Apr 27, 2017

Uh oh!

kratiahuja commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kratiahuja Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kratiahuja commented Apr 2, 2017 •

edited

Loading

kratiahuja commented Apr 2, 2017 •

edited

Loading

kratiahuja commented Apr 15, 2017 •

edited

Loading

kratiahuja Apr 26, 2017 •

edited

Loading

simonihmig Apr 26, 2017 •

edited

Loading

kratiahuja Apr 27, 2017 •

edited

Loading

kratiahuja commented Apr 27, 2017 •

edited

Loading

kratiahuja Apr 28, 2017 •

edited

Loading

stefanpenner commented May 5, 2017 •

edited

Loading

stefanpenner commented May 5, 2017 •

edited

Loading

stefanpenner commented May 15, 2017 •

edited

Loading