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

Revisiting the visit API (aka FastBoot™) #12394

Merged
merged 21 commits into from Sep 30, 2015

Conversation

Projects
None yet
8 participants
@chancancode
Member

chancancode commented Sep 26, 2015

↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️ ↪️

It's time to put on some finishing touches for the visit API! Since this is a somewhat big patch, here is a detailed description.

Summary

The visit API exposes ApplicationInstance as a useful primitive for many scenarios, including FastBoot™, widgets, resource discover, testing, and more.

Background

Ember 1.12 introduced the concept of ApplicationInstances. While they are very well designed and could be very useful in many cases, there are no official APIs to create them. The visit API helps fill this void by exposing a supported way to boot and customize instances.

What's in the box 📦

  • Refactoring
  • New APIs
  • Tests
  • Documentation
  • Bonus DLC Pack
  • Emojis

Refactoring

  1. A Clear Boot Phase

    Previously, we don't have a very clear concept of a boot phase, especially for ApplictionInstance. This creates a lot of implicit timing dependencies around when certain options can be set/changed. For example, the ember-cli-fastboot changes the autoboot flag in an application initializer (i.e. in the middle of the boot phase), which created a strange fork in the boot flow.

    As part of this patch I did some work to solidify the boot phase. Options like autoboot should be set
    before boot() is called at instantiation time. What I can do on Application is rather limited, but I tried to make that clear in the new ApplicationInstance boot phase, which takes a hash of boot-time options in favor of ad-hoc methods like overrideRouterLocation.

  2. Asynchronous APIs

    Booting an Application (and to a lesser extent, an ApplicationInstance) is an inertially asynchronous task. Naturally, we would like to express the asynchrony here with promises/async functions. However, the architecture we have predated Ember's adoption of promises, and a lot of code assumes that it is a "synchronous". Specifically, a lot of tests assumes the last call to app.advanceReadiness() or app.reset() will complete the boot process when the same runloop ends, which makes it impossible to async-ify the process.

    This wasn't clear to me when I started working on the code – I actually spent some time making them fully async, only to find a lot of tests failing. However, we should start embracing the asynchrony here going forward. I converted the primary (internal) APIs async, which we can start using for new code like the visit API. Internally, they wrap their synchronous counterparts which we will use for legacy paths.

    This makes things a bit annoying in the short term, but it should help us iterate towards the right direction.

  3. Globals Mode

    Previously, setting the autoboot flag also skipped some seemingly unrelated code paths in the boot process. It turns out that most of these code paths are there for the legacy "globals mode" (where the application object acts as a "namespace" for attaching classes, i.e. App.MyComponent = ...).

    While it's fair to overload the autoboot flag as an opt-out for these features, they are ultimately unrelated concerns. To make the distinction clear, I added an internal _globalsMode flag to track these code paths. Its value is currently tied to the autoboot flag, but you can manually hardcode the autoboot = true, _globalsMode = false combo and run the tests, which should reveal that spots where we are relying on these behaviors. Since most of these features have already been deprecated, separating it from autoboot as a concept should help to review and remove them independently.

  4. Expansion of the environment concept

    With HTMLBars, most of the initial render path has already been jQuery-free (which is why FastBoot works in node). The last bit I had to plumb was the view.appendTo(rootElement) call, which uses jQuery to resolve the rootElement selector into an Element.

    The current visit implement works around this by exposing the "root view" without attaching its element. This isn't ideal since we already deprecated/removed views and it requires the use of other private APIs like appendTo and renderToElement.

    In practice, if we were given an actual Element object as the rootElement, our renderer already knows how to append it into the DOM without jQuery. All I need to do is to tell plumb that through to the view layer so that it can skip the jQuery lookup if the user has opt-ed to not user jQuery. To do this, I expanded on the environment abstraction and made it instance specific through the -environment:main container key. I only did what I needed to do to make my scenarios work, but that should lay the ground work for the eventual "optional jQuery" world.

See also #11291 and #11289 (comment)

New (Public) APIs

  • Application.autoboot flag
  • Application#boot()
  • Application#visit()
  • ApplicationInstance#getURL()
  • ApplicationInstance#visit()

Tests

I expanded the unit tests coverage, and added some end-to-end integration tests for the a few target scenarios we intend to support.

Documentation

You can find them in the diff.

Bonus DLC Pack

To redeem, enter the following code into the Ember Data store: F83J-7BD2-D16Z-3QA4

Emojis

👍 ⭐️ 👉 ❤️ 🎧 🔑 🚀 👺

Differences from current implementation

  • Application.autoboot should only be set before boot time (e.g. at create time)
  • Application#visit() no longer rejects if a redirect occurred during the initial transition
  • Application#visit() no longer attach a view property to the instance it returns
  • Application#visit() no longer prevents the router from inserting the top-level view into the DOM (use rootElement to customize the insertion point as usual)
  • Application#visit() resolves only after rendering is complete and the root view is attached to the DOM
  • Application#visit() now takes an option hash for customizing the instance
  • Uncaught errors in application/instance initializers now causes Application#visit() to reject instead of synchronously throws
  • Removed ApplicationView#overrideRouterLocation (use the location boot option instead)
  • New flags that enables a much wider range of scenarios
  • Better documentation

TODOs

  • Add some node tests
  • Update the ember-cli-fastboot implementation
  • Move ember-cli-fastboot-specific hacks into the appropriate package; since the DOMHelper API is not public, we cannot ship until things works out of the box (see also krisselden/morph-range#7)
  • Better error reporting: differentiate between a BootError and a TransitionError: for transition errors, it is useful to still return the instance, as there we might have rendered an error page into the DOM

Open questions

  • Various TODOs in the code comments that I have no answers to
  • Are we comfortable committing the the three scenarios I listed in the documentation? Do we need to add more?
  • Is the ApplicationInstance#visit implementation correct?
  • Can we consolidate autoboot with Ember CLI's autorun flag? (they do related but not identical things)

Future extensions

  • Formalize the API requirements for the fake DOM, DOMHelper, etc
  • Use these API for tests: boot a new instance in each test case by calling visit() and destroy() it in teardown, instead of calling App.reset(). Use ApplicationInstance#visit to implement the visit test helper, etc
  • Work with @mitchlloyd on Ember Islands type scenarios (see the test)
  • Flesh out the options to enable other scenarios
  • Plumb the environment through the rest of the view layer to make jQuery optional
  • Remove reliance on globals mode in tests
  • Add a mechanism to differentiate between the various types of redirects and common errors, so that a FastBoot server can return the right status code

cc @wycats @tomdale @stefanpenner @krisselden @mitchlloyd @chadhietala @joshvfleming 🙏

chancancode added some commits Sep 23, 2015

Untangle autoboot and globalsMode code paths
Conceptually, these are two orthgonal things – autoboot happens to be
the opt-in for disabling support for globals mode.

After this refactor, autoboot is literally just calling `boot()` at
DOM Ready and *then* creating a instance automatically for you.
Conversely, you mustcall `boot()` yourself if you disable autoboot).

Most of the globals mode featue are already deprecated in 1.x.
Separating the two concepts allow us to review and remove those
feaatures independently.

Locally overriding the flags to use the `(autoboot: true, globalsMode:
false)` combo and running the tests would reveal all the places we are
relying on them internally.
Refactor application (instance) boot paths
This is a dialed back attempt at making the `ApplicationInstance`
boot process fully async. I wrote all the code to make that happen,
only to find out that a lot of tests implicitly depend on the process
being synchronous. So, I did the following instead:

1. Create an async version of `boot` on both `Application` and on
   `ApplicationInstance`. Going forward, new APIs (`visit`, etc)
   should use the async version.

2. Keep an internal `_bootSync` method on both classes and use them
   in places where we rely on the process being synchronous.

In the short term, having to support both synchronous and
asynchronous might make things look a bit more complicated, but it
will help us iterate towards where we want to be.

Separately, this commit also solidified the concept of a `boot`
phase on `ApplicationInstances`. Instead of overriding things on
the instance in an ad-hoc manner and implicitly rely on the timing
of these calls (e.g. `overrideRouterLocation` must be called before
the router is instantiated/cached), they have now been consolidated
into options that you pass into the `boot()` method.

Having an explicit boot phase makes it clear what can and cannot
happen at runtime.

(TODO: move the "start routing" logic into the boot phase)
@@ -79,7 +87,9 @@ let ApplicationInstance = EmberObject.extend(RegistryProxy, ContainerProxy, {
var application = get(this, 'application');
set(this, 'rootElement', get(application, 'rootElement'));
if (!isEnabled('ember-application-visit')) {

This comment has been minimized.

@tomdale

tomdale Sep 26, 2015

Member

Why is using the feature flag? Application instances aren't exposed via public API if the flag is disabled, right?

@tomdale

tomdale Sep 26, 2015

Member

Why is using the feature flag? Application instances aren't exposed via public API if the flag is disabled, right?

This comment has been minimized.

@chancancode

chancancode Sep 26, 2015

Member

They can't be created manually via public API, but they are used by Ember. Basically this is just trying to be carefully are preserve the /exact/ point where this was happening (I moved this exact call down into the boot method), I will be happy to unify them below if that's fine with you

@chancancode

chancancode Sep 26, 2015

Member

They can't be created manually via public API, but they are used by Ember. Basically this is just trying to be carefully are preserve the /exact/ point where this was happening (I moved this exact call down into the boot method), I will be happy to unify them below if that's fine with you

var location = options && options.location;
var router = get(this, 'router');
_bootSync(options) {
if (this._booted) { return this; }

This comment has been minimized.

@tomdale

tomdale Sep 26, 2015

Member

Are there valid use cases for absorbing a double call to _bootSync? I can imagine that if the second call has different options (that are thus ignored) and there's no error, tracking that bug down will be extremely annoying.

@tomdale

tomdale Sep 26, 2015

Member

Are there valid use cases for absorbing a double call to _bootSync? I can imagine that if the second call has different options (that are thus ignored) and there's no error, tracking that bug down will be extremely annoying.

This comment has been minimized.

@chancancode

chancancode Sep 26, 2015

Member

Hm, I was just mirroring the semantics of the async path. Maybe calling boot with options, on an already booted instance should result in a rejection? Or would that just be a normal assertion (exception)?

@chancancode

chancancode Sep 26, 2015

Member

Hm, I was just mirroring the semantics of the async path. Maybe calling boot with options, on an already booted instance should result in a rejection? Or would that just be a normal assertion (exception)?

* It sets the `location` option to `"none"`. (If you would like to use
the location adapter specified in the app's router instead, you can also
specify `{ location: null }` to specifically opt-out.)

This comment has been minimized.

@tomdale

tomdale Sep 26, 2015

Member

👍 This cleanup is great.

@tomdale

tomdale Sep 26, 2015

Member

👍 This cleanup is great.

throw new Error(error.message);
} else {
throw error;
}

This comment has been minimized.

@chancancode

chancancode Sep 29, 2015

Member

I'm not 100% sure this is correct, someone pointed out to me that using router.router.activeTransition might be (incorrectly) relying on sync behavior. (Although I see other places using it too.)

However, since the docs/tests is describing the intended behavior (it should follow all redirects and then resolve when rending work is complete, if any), and this implementation is correct as far as the current tests are concerned, if no one has any immediate suggestions this shouldn't be a blocker either. If someone eventually discovered the theoretical bug we can change the implementation to match the intended semantics.

@chancancode

chancancode Sep 29, 2015

Member

I'm not 100% sure this is correct, someone pointed out to me that using router.router.activeTransition might be (incorrectly) relying on sync behavior. (Although I see other places using it too.)

However, since the docs/tests is describing the intended behavior (it should follow all redirects and then resolve when rending work is complete, if any), and this implementation is correct as far as the current tests are concerned, if no one has any immediate suggestions this shouldn't be a blocker either. If someone eventually discovered the theoretical bug we can change the implementation to match the intended semantics.

chancancode added some commits Sep 29, 2015

Merge branch 'master' into visit_api
Conflicts:
  packages/ember-application/lib/system/application.js
  packages/ember-application/lib/system/application-instance.js
  packages/ember-application/tests/system/visit_test.js
  packages/ember-views/lib/mixins/view_support.js
Rollback a few non-critical changes to derisk
The gets the unflagged branches more closer aligned with master; most
important one being `boot` is back to `@Private` again since there are
no real reason you need to call it (`visit` calls it automatically), and
because we cannot feature flag docs (and have one of them public, the
other one private).
@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Sep 30, 2015

Member

The public API here is not final yet, but it's getting close.

It replaces older unstable code behind a flag, so I'm going to merge it so that people who are experimenting with the visit() API in Canary can continue to provide feedback, and we can hopefully stabilize this for 2.3.

Member

wycats commented Sep 30, 2015

The public API here is not final yet, but it's getting close.

It replaces older unstable code behind a flag, so I'm going to merge it so that people who are experimenting with the visit() API in Canary can continue to provide feedback, and we can hopefully stabilize this for 2.3.

wycats added a commit that referenced this pull request Sep 30, 2015

Merge pull request #12394 from chancancode/visit_api
Revisiting the visit API (aka FastBoot™)

@wycats wycats merged commit ab1f9df into emberjs:master Sep 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@btecu

This comment has been minimized.

Show comment
Hide comment
@btecu

btecu Sep 30, 2015

Contributor

Does this file belongs in the repo? Seems to be very specific to an editor.

Contributor

btecu commented on .vscode/settings.json in 00fc5a5 Sep 30, 2015

Does this file belongs in the repo? Seems to be very specific to an editor.

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Sep 30, 2015

Member

omg! I am so sorry :( I'll fix it soon, thanks for catching it!

Member

chancancode replied Sep 30, 2015

omg! I am so sorry :( I'll fix it soon, thanks for catching it!

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 3, 2016

Member

@chancancode when is it the case that we don't have an environment here?

I suspect we should always have an environment here, and if a scenario exists where that is not currently true, and not currently a bug, we should pass in a "null pattern environment". The existence of this guard I believe hid the following "engine + visit + shouldRender: false" bug -> #14015

Member

stefanpenner commented on packages/ember-routing/lib/system/route.js in 8e317ee Aug 3, 2016

@chancancode when is it the case that we don't have an environment here?

I suspect we should always have an environment here, and if a scenario exists where that is not currently true, and not currently a bug, we should pass in a "null pattern environment". The existence of this guard I believe hid the following "engine + visit + shouldRender: false" bug -> #14015

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Aug 4, 2016

Member

It was probably some funky test setup, if removing it doesn't cause any tests to fail, please go ahead 👍

Member

chancancode replied Aug 4, 2016

It was probably some funky test setup, if removing it doesn't cause any tests to fail, please go ahead 👍

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 4, 2016

Member

@chancancode going forward, lets avoid this if possible. I'll see if I can remove.

Member

stefanpenner replied Aug 4, 2016

@chancancode going forward, lets avoid this if possible. I'll see if I can remove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment