[BUGFIX stable] Instance initializers should run before App.ready

[chancancode]

The original complaint is that routing begins before instance initializers are run, which means that some routes instances are created before the instance initializers are run, which means that you cannot reliably setup injections for routes in instance initializers, which are important for e.g. store:main.

Since App.ready is guaranteed to run before routing begins, this effectively guarantees that instance initializers are run before routing begins.

Fixes #11172

[chancancode]

See line 186 ⬆️

[rwjblue]

Thanks for tackling this! Can you add a test that uses autoboot: false (which is how fastboot works) and confirms the ordering of ready/runInstanceInitializers?

[chancancode]

I was hoping no one would notice ;)

autoboot is more tricky. TL;DR, the feature is pretty buggy/broken atm, and only more or less works with the exact implementation in ember-cli-fastboot currently. For example...

Clearly, autoboot is meant to be set before the application instance is created, because the value is used in the constructor. But if this is the case, then domReady (hence boot) is actually never called, so the application initializers and application load hooks are actually never run 💣

didBecomeReady is also never reached (unless you have an unbalanced number of advanceReadiness calls :P), I'm not 100% sure what the implications are yet 😄

The ready hook actually doesn't fire for autoboot: false either, which might be okay, because the definition of the hook is fires immediately before (automatic?) routing and it's fired on the Application (not ApplicationInstance), so presumably it only applies to the default instance (?).

---

But anyway, ember-cli-fastboot doesn't actually have most of these problems, because it actually changes the autoboot flag in an application initializer currently, so it's already half-way in the booting process. The ready hook still doesn't fire, but like I said it's probably expected (but most of didBecomeReady still doesn't run). 😄

If you are using the visit entry point like ember-cli-fastboot does (and I think that's the only semi-supported entry point for autoboot: false, but I could be wront), then it definitely doesn't have this bug currently:

ember.js/packages/ember-application/lib/system/application.js

Lines 821 to 822 in a0acc6e

	var instance = this.buildInstance();
	this.runInstanceInitializers(instance);

. Clearly the instance initializers are run before routing starts there.

So I think I'll (or someone else 😉) need to do a lot more cleanup for autoboot anyway, and adding a test for this case at the moment would be pretty meaningless considering all the other different ways it could break. (The test would be an integration test that maps 1:1 to the current "special" implementation in the addon currently.)

I have other complains/fixes/suggestions for ember-application-visit, more to come later (but we definitely can't ship this current version 😄).

[rwjblue]

I have come to similar conclusions regarding the manual boot capability and fastboot, I mostly asked for the test because it seems currently broken and I want to make sure that the fix here (for the autoboot case) doesn't make that problem worse.

However, ember-application-visit is a flagged feature and we should definitely ensure that golden path works properly, so 👍 on this for now. We really need to get the autoboot: false case fixed though...

[chancancode]

Just to be clear, #11172 does not affect autoboot: false, as far as I can tell. It has a different code path that ensures the instance initializers are called before routing begins on each instance (at least if you use App#visit, again I don't know of other "supported" ways to build an instance).

If I am wrong then we should probably add the test even temporarily 😁

[rwjblue]

@chancancode - You are not wrong.

My concern was that this changes the way an app boots (and definitely fixes #11172), which does in some way affect autoboot: false case. We probably need to make a meta issue to track the issues that we have discovered with ember-application-visit so that we don't loose track of getting those fixed...

[chancancode]

👍

[jayphelps]

@rwjblue Can this be cherry-picked as a 1.12.1 to unblock ember-data so they can remove the final deprecations?

(I'm lurking, so not sure but this sounded like a regression, not a new feature that was buggy. Please correct me if I'm wrong)

[ppcano]

I agree with @jayphelps, releasing 1.12.1 would be nice to remove those deprecations.

[rwjblue]

@jayphelps - It was already cherry picked into release branch when you commented 💃, 1.12.1 will be released as soon as we get the rootURL issue fixed.

Also, instanceInitializers were a new feature in 1.12, using a "normal" initializer has continued to function properly but results in a deprecation. I personally do not think that an errant deprecation is a regression.

[mitchlloyd]

Not sure where a good place to drop this is, but seeing @chancancode imply that he MIGHT do some cleanup work around the boot process made we want to add some thoughts. These ideas are not RFC ready, but I feel like there should be a public API for booting Ember applications that includes;

• How/if to setup an instance of an Ember.Application (not sure about this one)
• How/if to build the default registry
• How/if to run initializers
• How/if to setup the router
• How/if to setup the eventDispatcher

I know the autoboot: true/false is just the first crack at getting FastBoot working, but long term it seems like something more flexible would be nice. For instance, in ember-islands I had a feature request that needed an Ember app booted without the router. I put in a horrible hack, and would love something better there.

At a high level here are some situations I'm thinking about. Obviously this is all pseudocode, and needs some in-depth spelunking and more thought.

// I wanted this for my addon
bootstrapRegistry();
runInitializers();
// startRouting(); no routing please
startEventDispatcher();

// I think FastBoot wants this
bootstrapRegistry();
runInitializers();
startRouting();
// startEventDispatcher(); no events needed

// An integrated component test might want
bootstrapRegistry();
// app.runInitializers(); no initializers please
// app.startRouting(); no routing needed
app.startEventDispatcher();

[chancancode]

@mitchlloyd I added it to here! #11291 (comment)