Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

App Boot Hooks #573

Closed
wants to merge 7 commits into from
Closed

App Boot Hooks #573

wants to merge 7 commits into from

Conversation

mehulkar
Copy link
Contributor

@mehulkar mehulkar commented Jan 10, 2020

@gossi
Copy link

gossi commented Jan 10, 2020

Sounds like something I wrote about earlier, read here: https://gos.si/blog/thoughts-on-improving-developer-ergonomics-for-emberjs/

Since stages of app boot are unclear to me, I was wondering whose those are and at which your methods would be called. One regarding the initializing process (and having control over the sequence here), second is also what currently goes into ApplicationRoute.beforeModel() hook. Stuff like loading ember-intl and setting it's language. Would love to see more details on this.

Second, I'm strongly against on* methods. It is application.setup() rather than application.onBoot(). The first not only tells a better story, the method is an active part not a reactive one.

@mehulkar
Copy link
Contributor Author

Second, I'm strongly against on* methods. It is application.setup() rather than application.onBoot(). The first not only tells a better story, the method is an active part not a reactive one.

I'm ok with a different name, but the part that we're trying to hook into is called boot() already. I was going for a name that said "this is related to boot, but we're not guaranteeing exactly where in boot it runs"

second is also what currently goes into ApplicationRoute.beforeModel() hook.

💯. This is one of the ergonomic problems I'd like to solve, by way of this RFC and #572. Currently, the deciding factor usually ends up being "is this code sync or async", rather than "does this belong in a Route or in app initialization". If #572 is accepted and we can align the feature set between the two places, I think we'll find ourselves in a better position to really answer this question. ApplicationRoute beforeModel is currently a shoehorn for every promise resolution that needs to happen early in the app, and I think we can improve that.

@gossi
Copy link

gossi commented Jan 11, 2020

I'm ok with a different name, but the part that we're trying to hook into is called boot() already . I was going for a name that said "this is related to boot, but we're not guaranteeing exactly where in boot it runs"

Here are a few: preBoot(), postBoot() rsp beforeBoot(), afterBoot() =)

@mehulkar
Copy link
Contributor Author

Here are a few: preBoot(), postBoot() rsp beforeBoot(), afterBoot()

I'm open to this, but we'd have to first define "boot". If we simply called postBoot at the end of the existing boot method, we might introduce a problem because initializers would run after the _booted flag is set.

Also, the major reason for these hooks (for me anyway), is to have a place to put initializers. Since the "post/after" version of the hook is the thing that matters, I'm not sure if there is enough justification to have a "pre" version (there might be some use cases for non autobooted apps though) 🤔.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely agree that declarative hooks, where you directly import and run your "initialization" code is much better than the current initializer system (and would be quite a bit faster).

I think the conceptual goal of this RFC is good, but I do not want to land a change to how initializers work only to subsequently deprecate them (which is what I'd like to do).

I'm personally not sold on the names onBoot/onBootInstance, but naming is hard and it doesn't seem like you are particularly tied to a specific name (e.g. if we come up with a better name, we can update to that).

@mehulkar
Copy link
Contributor Author

mehulkar commented Feb 8, 2020

but I do not want to land a change to how initializers work only to subsequently deprecate them

  1. This wouldn't change how they work! It would just mean exposing the point at which they run so that uses can replace them if they want. If anything, I think it'll make it easier to deprecate.
  2. Deprecating initializers would mean that they are replaced by something right? Is manual boot the correct replacement then? Or do you not anticipate any need to yield to the app after creating an Application/ApplicationInstance, but before starting routing?

@mehulkar
Copy link
Contributor Author

@rwjblue any additional thoughts about this one?

@xg-wang
Copy link
Contributor

xg-wang commented Apr 16, 2021

Can you add more usecases / examples to explain how one would use the new APIs?

And if I don't follow the default:

import { loadInitializers, loadInstanceInitializers } from 'ember-load-initializers';
import Application from '@ember/application';
import config from './config/environment';
export default class App extends Application {
    onBoot() {
        loadInitializers(this, config.modulePrefix);
        this.runInitializers();
    }
    onInstanceBoot(appInstance) {
        loadInstanceInitializers(this, config.modulePrefix)
        this.runInstanceInitializers();
    }
}

Will any of the addon provided initializer not run? In that case how would an addon author communicate with the app consumer?

In some cases, this is too late. For example, if an application needs to wait for an event
from its host environment e.g. an iframe or an electron shell, and use it to determine
the starting URL, it cannot do so.
- **First Transition**: Similarly, if one wants to add an event listener to the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's discussion first transition should be handled by an instance-initializer
#680 (comment)

Will there be any recommendation change after this RFC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be handled by a function that runs in boot() instead. Note that this RFC doesn't fundamentally change the fact that you can run code during application boot. It just changes the developer ergonomics to enable new features.

By providing a way to hook into the existing, async boot sequence more directly,
application developers can write declarative, explicit code about how they want to
customize their application.
- **Blocking async code before routing**. Currently, the earliest place an application can write
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this currently achieved by deferReadiness? Can you describe what would the new alternative look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deferReadiness is not available for instance initializers and it's an awkward API to begin with when we could just use promises.

@mehulkar
Copy link
Contributor Author

Will any of the addon provided initializer not run? In that case how would an addon author communicate with the app consumer?

import { loadInitializers, loadInstanceInitializers } from 'ember-load-initializers';

This would do the same thing that currently happens, which involves gathering up all the files in the app and addons initializers/instance-initializers directories.

Co-authored-by: Clemens M�ller <[email protected]>
@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

What's the status of this? Are we just waiting on more review?

@mehulkar
Copy link
Contributor Author

I still believe this is a good idea and the right abstraction for an ember application. I am no longer working primarily in Ember though, so I’m not sure how I’ll be able to keep contributing. If any core team feedback appears, I would be happy to address it though.

@wagenet wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022
@mehulkar mehulkar closed this Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Proposed In the Proposed Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants