Prepare for FastBoot 1.0 by simonihmig · Pull Request #21 · ember-fastboot/ember-cli-head

simonihmig · 2017-04-26T20:16:21Z

This PR adds changes to make the addon compatible with FastBoot pre 1.0 and post 1.0 (no instance-initializers/(browser|fastboot). Also added FastBoot tests (currently only running with FastBoot pre 1.0)

cc @kratiahuja

kratiahuja · 2017-04-26T23:08:07Z

cross referencing so i can review later: ember-cli-fastboot issue

kratiahuja · 2017-04-27T00:57:22Z

index.js

-    var checker = new VersionChecker(this);
-    var emberVersion = checker.for('ember-source', 'npm');
+  treeForApp(defaultTree) {
+    let checker = new VersionChecker(this);


My PR exposes a process environment process.env.FASTBOOT_NEW_BUILD and addToFastBootTree for these cases. Instead of merging here you can just do this:

Let me know if this doesn't work or you feel this may not be needed but I exposed those hooks in my PR to have this in mind. More than happy to drop code 😄

treeForApp(defaultTree) { if (!process.env.FASTBOOT_NEW_BUILD) { var emberVersion = checker.for('ember-source', 'npm'); if (!emberVersion.version) { emberVersion = checker.for('ember', 'bower'); } var trees = [defaultTree]; // 2.9.0-beta.1 - 2.9.0-beta.5 used glimmer2 (but 2.9.0 did not) // 2.10.0-beta.1+ includes glimmer2 if (!(emberVersion.gt('2.9.0-beta') && emberVersion.lt('2.9.0')) && !emberVersion.gt('2.10.0-beta')) { trees.push(this.treeGenerator(path.resolve(this.root, 'app-lt-2-10'))); } var tree = mergeTrees(trees, { overwrite: true }); return filterInitializers(tree); } else { return defaultTree; } } addToFastBootTree() { // this hook will be invoked in post fastboot 1.0 (after my PR is merged) var fastbootBuilder = require('ember-cli-fastboot/lib/build-utilities/fastboot-builder'); let checker = new VersionChecker(this); let emberVersion = checker.for('ember-source', 'npm'); if (!emberVersion.version) { emberVersion = checker.for('ember', 'bower'); } // 2.9.0-beta.1 - 2.9.0-beta.5 used glimmer2 (but 2.9.0 did not) // 2.10.0-beta.1+ includes glimmer2 if (!(emberVersion.gt('2.9.0-beta') && emberVersion.lt('2.9.0')) && !emberVersion.gt('2.10.0-beta')) { return fastbootBuilder.addFastBootPath(path.resolve(this.root, 'fastboot-app-lt-2-9')); } }

When FastBoot 1.0 is released, we can remove treeForApp.

simonihmig · 2017-04-27T20:58:58Z

Fyi: added additional FastBoot tests, that now run pre and post FB 1.0 scenarios. That should now cover all cases, including pre-Glimmer2.

simonihmig · 2017-04-27T21:06:24Z

@kratiahuja Just pushed some changes, is that what you had in mind?

The problem here is that tests will be failing. Because ember-cli-addon-tests will symlink the addon, the require('ember-cli-fastboot/lib/build-utilities/fastboot-builder') cannot be resolved (as it is just a dependency of the consuming app, not of the addon). This is an issue for the tests, but also all cases where the user symlinks the addon (npm link)...

kratiahuja

Can you try running the tests against ember-cli-fastboot master if possible? It should pass with those. If we can get it to run against master then we can do a release for that (rc.1) and then upgrade here.

kratiahuja · 2017-05-18T02:43:02Z

index.js

+  },

-    var trees = [defaultTree];
+  addToFastBootTree() {


This is now called treeForFastBoot per the latest.

kratiahuja · 2017-05-18T02:44:18Z

index.js

-    var trees = [defaultTree];
+  addToFastBootTree() {
+    // this hook will be invoked in post FastBoot 1.0
+    let fastbootBuilder = require('ember-cli-fastboot/lib/build-utilities/fastboot-builder');


You don't need this any longer. treeForFastBoot now takes a treeGenerator. See an example here

…stboot#master

simonihmig · 2017-05-19T16:33:33Z

@kratiahuja Updated this according to your suggestions!

kratiahuja

Assuming you have tested all the three scenarios listed here: https://gist.github.com/kratiahuja/d22de0fb1660cf0ef58f07a6bcbf1a1c#testing-guidelines, this LGTM

kratiahuja · 2017-05-21T21:30:55Z

fastboot-lt-2-10/instance-initializers/fastboot/head.js

@@ -15,5 +15,5 @@ export function initialize(instance) {

 export default {
  name: 'head-fastboot',
-  initialize: initialize


The initialize function needs to be wrapped in a if (typeof FastBoot != 'undefined') {...} check to make sure ember-cli-head works in apps without ember-cli-fastboot.

Just had a look at it again... I think this is not needed, as this is not part of app

for apps that are not using the new FastBoot build (beta FastBoot or not FB at all), this is merged into app through filterInitializers (https://github.com/ronco/ember-cli-head/pull/21/files#diff-168726dbe96b3ce427e7fedce31bb0bcR23), so the browser build should not include this

for apps having the new FastBoot, this is only added to treeForFastboot

And the ember 2.4 test scenario seems to be ok!

Sounds good. Missed the filterInitializers again 👍

simonihmig · 2017-05-21T22:52:05Z

Assuming you have tested all the three scenarios listed here

I'll give this another test drive tomorrow...

simonihmig · 2017-05-23T08:41:26Z

There was one missing piece (merging app-lt-2-10 for pre-Glimmer2 and new FastBoot) which I fixed now.

I think this should be fine now and ready for final review/merge! cc @ronco @rwjblue

kratiahuja

LGTM

kratiahuja · 2017-05-24T04:34:45Z

index.js

+
+      return filterInitializers(tree);
+    } else {
+      let trees = [defaultTree];


Can't you do this in treeForFastBoot?

But that's just for the browser. That's covering the new FastBoot, but with an pre 2.10 ember, where we need to merge the app-lt-2-10 browser initializer!

stefanpenner · 2017-05-26T23:05:57Z

@rwjblue i believe you may have commit/release, mind sharing it? So we can fix/release when this is ready.

rwjblue · 2017-05-26T23:29:36Z

Ya, will do once I am back at a computer

mazondo · 2017-05-28T20:21:39Z

I'm seeing an issue with this where instance initializer head-browser is injected twice and throws an error if you're using fastboot but pass ?fastboot=false as a param.

Edit: Actually, this is happening all the time. Going to dig into it now.

mazondo · 2017-05-29T02:30:19Z

There were a few things going on in my case that caused the error, mostly interactions between ember-page-title, ember-cli-meta-tags and ember-cli-head.

Edit: removing ember-page-title and ember-cli-meta-tags resolved this issue for me.

kratiahuja · 2017-06-01T05:42:32Z

Ping @rwjblue can we merge and release this?

rwjblue · 2017-06-01T13:14:56Z

package.json

    "ember-cli-htmlbars": "^1.3.0",
    "ember-cli-version-checker": "^1.1.6",
-    "fastboot-filter-initializers": "0.0.1"
+    "fastboot-filter-initializers": "^0.0.2"


Caret here does nothing, we should bump to 0.1.0 (upstream) so that we can use caret deps...

rwjblue · 2017-06-01T13:17:12Z

@kratiahuja / @simonihmig - What should the version bump be? Seems to be a compatible change (lots of effort for backwards compat in this PR), so I would assume 0.2.1 but I want to double check...

simonihmig · 2017-06-01T13:22:47Z

@rwjblue yeah, it got a bit more complicated because of trying to be compatible with FastBoot beta-series as well as 1.0 (rc). But this dual compatibility is covered by those separate FastBoot tests, so I'm pretty confident that this should not break anything (hopefully!). So 0.2.1 should be ok!

rwjblue · 2017-06-01T13:24:55Z

ember-cli-head 0.2.1 published 🎉

simonihmig added 2 commits April 26, 2017 21:32

Add FastBoot tests

abefcb7

Fix for FastBoot 1.0

8777d6a

kratiahuja reviewed Apr 27, 2017

View reviewed changes

simonihmig changed the title ~~Prepare for FastBoot 1.0~~ [WIP] Prepare for FastBoot 1.0 Apr 27, 2017

Added FastBoot 1.0 tests, fixed Travis

f4e7b7b

Use addToFastBootTree hook

e7fb768

This was referenced Apr 27, 2017

Refactor fastboot build to be more performant ember-fastboot/ember-cli-fastboot#369

Merged

Use preconcatTree hook for filterInitializers #24

Closed

simonihmig mentioned this pull request May 17, 2017

Add-ons Prep for fastboot 1.0.0 ember-fastboot/ember-cli-fastboot#387

Closed

kratiahuja reviewed May 18, 2017

View reviewed changes

Use new treeForFastBoot hook, run FastBoot tests against ember-cli-fa…

3b2ae51

…stboot#master

kratiahuja reviewed May 21, 2017

View reviewed changes

simonihmig mentioned this pull request May 21, 2017

Add testing section to addon guide ember-fastboot/fastboot-website#64

Open

kratiahuja mentioned this pull request May 22, 2017

Unskip ember-cli-head usage test ember-fastboot/ember-cli-fastboot#393

Closed

Fix missing merge of app-lt-2-10 for new FastBoot build

a31877f

simonihmig changed the title ~~[WIP] Prepare for FastBoot 1.0~~ Prepare for FastBoot 1.0 May 23, 2017

Fix FastBoot tests

022a6e7

kratiahuja approved these changes May 24, 2017

View reviewed changes

buschtoens mentioned this pull request May 24, 2017

make page-title-setup initializer fastboot compatible ember-cli/ember-page-title#70

Merged

mschinis mentioned this pull request May 24, 2017

Support ember-cli-fastboot rc1 ember-learn/ember-api-docs#222

Closed

buschtoens mentioned this pull request May 24, 2017

RC1 broke ember-cli-meta-tags ember-fastboot/ember-cli-fastboot#402

Closed

kratiahuja mentioned this pull request May 27, 2017

Unskip ember-cli-head test ember-fastboot/ember-cli-fastboot#407

Merged

1 task

rwjblue reviewed Jun 1, 2017

View reviewed changes

rwjblue merged commit b3bfcbc into ember-fastboot:master Jun 1, 2017

This was referenced Jun 1, 2017

Support FastBoot 1.0 ronco/ember-cli-meta-tags#55

Closed

Fix duplicate initializers appearing in dist js file #28

Closed

kratiahuja mentioned this pull request Jun 4, 2017

Fastboot deprecation #29

Closed

This was referenced Jun 5, 2017

Make addon more idiomatic at the expense of making it 2.10+ & fastboot 1.0rc+ #32

Merged

head-browser instance-initializer not found from ember-page-title, booting the app fails (v0.3, fastboot rc3) #34

Closed

Comments

Conversation

simonihmig commented Apr 26, 2017

Uh oh!

kratiahuja commented Apr 26, 2017

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!

simonihmig commented Apr 27, 2017

Uh oh!

simonihmig commented Apr 27, 2017

Uh oh!

kratiahuja left a comment

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!

simonihmig commented May 19, 2017

Uh oh!

kratiahuja left a comment • 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!

simonihmig commented May 21, 2017

Uh oh!

simonihmig commented May 23, 2017

Uh oh!

kratiahuja left a comment

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!

stefanpenner commented May 26, 2017

Uh oh!

rwjblue commented May 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

mazondo commented May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kratiahuja commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Jun 1, 2017

Uh oh!

simonihmig commented Jun 1, 2017

Uh oh!

rwjblue commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kratiahuja Apr 27, 2017 •

edited

Loading

kratiahuja left a comment •

edited

Loading

mazondo commented May 28, 2017 •

edited

Loading

mazondo commented May 29, 2017 •

edited

Loading