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

Promise based initializers #572

Closed
wants to merge 3 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions text/0000-promise-based-initializers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
- Start Date: 2020-01-09
- Relevant Team(s): Ember.js
- RFC PR: https://github.com/emberjs/rfcs/pull/572
- Tracking: (leave this empty)

# Promise-based Initializers

## Summary

This RFC proposes the ability to return a promise from an initializer or instance-initializer
and pause application boot until that promise resolves.

## Motivation

Currently, the earliest place that developers can write asynchronous code is in the ApplicationRoute.
For some use cases, this is not early enough, and it is useful to be able to run asynchronous code
before the application begins routing.

Choose a reason for hiding this comment

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

Do we have known use cases where the Application route "is not early enough"? I'm not doubting the truth of this statement, but having an example use case or cases would make it easier for me to wrap my head around how this would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! If you have a webview embedded in a native layer that sends you events (say when the native app is launched via deeplink). Capturing that event and operating on it is an asynchronous operation. If you wait till the ApplicaitonRoute, the Ember app has already started routing, so the only option to get to the desired route is redirect(). That's starkly different from what happens when the Ember app is initialized at a subpath in the browser, however and it can cause redirect hell if you receive more than one of these events.

Copy link
Member

Choose a reason for hiding this comment

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

Even accepting this case as gospel, I don't see why the changes proposed in this RFC would be needed. The specific case seems to be that you want to avoid autobooting your application (until these events have come back), that is already a supported operation (without changes AFAICT).

This feels to me to be introducing a new feature to Ember for what seems like a very very specific/targeted scenario for a single application (that already has argubly better solutions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

skipping autoboot is very much NOT the happy path, regardless of what the intention of the visit API was. The reason I introduced this API is because initializers are one of the few non-async code paths in Ember APIs today and it seems like a good thing to have them regardless of the use cases I've listed here.

We do have other use cases also:

  1. Making a network call that sets some cookies before we do anything else
  2. Initializing 3rd libraries that have async APIs (or make their own async calls)

This stuff can happen in the route, but I'd argue that not all of them should happen in a route, if we have the concept of an initializer. If initializers weren't a thing at all, I'd focus more on the manual boot scenario and improving that.


For example, when booting an Ember application in an embedded environment, it may be necessary to
listen for events from the environment to determine the initial state to determine the initial URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

would using the beforeModel in the application route work?

Copy link
Contributor Author

@mehulkar mehulkar Jan 10, 2020

Choose a reason for hiding this comment

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

For this particular use case, the application route is too late because there are subtle differences between what it means to start routing at a URL and what it means to redirect to a URL. When an Ember app runs in a browser, it does the former.

That said, I think promise based initializers will have benefits beyond just determining the initial state of the application.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't redirect from the application route though. It,s the parent of everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

redirecting is just a fancy word for saying " cancel the current transition and start a new one". You can intercept a transition in the ApplicationRoute that would have ended at /foo and and redirect to /bar, for example.


Another example may be to interact with 3rd party libraries that need to be initialized early but
have an asynchronous API.

## Detailed Design
Copy link
Member

Choose a reason for hiding this comment

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

Some important details (to me at least 😸) are not mentioned here:

  • What happens if you say you are after an intializer that is async, are you called after its promise resolves? The RFC should explicitly state this behavior.
  • What is the benefit of adding async: true (vs doing something like promise-map-series)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • yes
  • async: true was dreamed up for opt-in beahvior and to make it easier to sell :D. I don't know what promise-map-series is, but it sounds like wrapping each initializer in a promise and executing them in order? i'm also fine with that, but I was worried that could have performance implications or could make it possible for some other framework code microtasks to run in between initializers. If neither of those are concerns, I have no problems with it.


The `Application` and `ApplicationInstance` (extending from `Engine` and `EngineInstance`
classes respectively) are responsible for running initializers and instance initializers. Each
implements a method that loops through the loaded initializers and executes the exported function.

Initializer and Instance Initializer files each export an object with an `initialize` property
that references a function. For backwards compatibility, an `async` property should be added, and
Copy link
Contributor

Choose a reason for hiding this comment

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

If the initialize return type can be detected do we still need async? I think handling async initializers based on this option or the actual return type affects unresolved questions below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out. I debated over the need for this key too. The main reason it's useful is that you cannot detect the return type without running the initializer. The purpose of knowing earlier is to opt the user into the promise-based flow (the whole thing would need to be wrapped in a promise). Without any async declarations, existing apps would be able to retain the exact same behavior as they have today.

should default to `false` if omitted. If true (or truthy), the Application/ApplicationInstance would
expect a promise to be returned from the function and wait for the promise to resolve
before running the next initializer in the queue. The value resolved from the promise would be
ignored. Promises that reject would throw an exception in development and be caught in production
Copy link
Member

Choose a reason for hiding this comment

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

and be caught in production

What does this mean? If initialization of an application fails, I cannot reason about how to make that app functional again...

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's up to do the developer to determine that. For example, we make a network request at the beginning to set some cookies. Without those cookies the app is perfectly functional, but could have unintended consequences later. Catching errors in production would just be a failsafe for apps who opts into this feature, but doesn't think through that. I'd be fine with removing this part.

builds.

For example:

```js
// app/initializers/foo.js
export function initialize() {
return Promise.resolve();
}

export default {
initialize,
async: true
};
```

For maximum backwards compatibility, the boot sequence that calls these initializers could implement
an early check to see if any async initializers are defined and forgo all async handling.

If implemented, future RFCs could:

- change the default value of the `async` property to `true`.
- deprecate `deferReadiness` and `advanceReadiness` entirely.
Copy link
Member

Choose a reason for hiding this comment

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

👍👍 - Seems like a great thing to do when we deprecate initializers 😃

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's up to core team what order to do all this in. This RFC seemed like a good path to "just use JS" that didn't require changing the initializer paradigm!


## How we teach this

1. The guides would need to document the `async` key of the exported object from initializers and instance-initializers.
2. Some advanced logging to see when initializer functions are called may be useful.
3. Some information about how this relates to `deferReadiness` and `advanceReadiness` would be useful.
It's possible that deprecating these APIs as part of this RFC is appropriate, so there's a clear
signal that one is preferred over the other.

## Drawbacks

- There could be some performance concerns over introducing promises at boot time, but because this
is an opt-in feature, they can be avoided.

## **Alternatives**

- The existing `{defer|advance}Readiness` APIs could be introduced for instance-initializers as well.
Although promise-based async isn't exactly the same as this API, and there could be subtle differences
between both, it is unlikely that end-users would actually end up caring.

## **Unresolved questions**

- What happens if an initializer is marked as async but doesn't return a promise?
- What happens if a promise is created in an async initializer but not returned?
- Does this affect the algorithm that orders initializers based on `before` and `after` APIs?
- Does this affect the `boot` vs `_bootSync` of Application and ApplicationInstance at all and
should that be cleaned up *before* attempting this?