X Tutup
Skip to content

Long-form bootstrapping syntax allowing multiple apps, root components#3852

Closed
alxhub wants to merge 3 commits intoangular:masterfrom
alxhub:bootstrapping
Closed

Long-form bootstrapping syntax allowing multiple apps, root components#3852
alxhub wants to merge 3 commits intoangular:masterfrom
alxhub:bootstrapping

Conversation

@alxhub
Copy link
Copy Markdown
Member

@alxhub alxhub commented Aug 26, 2015

No description provided.

@alxhub alxhub added the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 26, 2015
@alxhub
Copy link
Copy Markdown
Member Author

alxhub commented Aug 26, 2015

This is ready for an initial review while I verify that it will actually build and run - I expect there will be some changes to get the tests passing.

@alxhub alxhub force-pushed the bootstrapping branch 2 times, most recently from 471766e to c4b5b59 Compare August 26, 2015 20:29
@alxhub
Copy link
Copy Markdown
Member Author

alxhub commented Aug 26, 2015

Having lots of issues with the tests but debugging is yielding good results. I'm a little worried about backwards compatibility with bootstrap(), I think it will suffer, but it is an architectural change.

tbosch
tbosch reviewed Aug 27, 2015
View reviewed changes
modules/angular2/src/core/application_common.ts Outdated
Copy link
Copy Markdown
Contributor

@tbosch tbosch Aug 27, 2015

Choose a reason for hiding this comment

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

I think this should be split up: Dont' call DynamicComponentLoader.loadAsRoot, but implement the logic that is in DynamicComponentLoader.loadAsRoot directly here:

  1. call Compiler.compileHost, which is asynchronous
  2. call ViewManager.createRootHostView with the result, which is synchronous

Could we have the new bootstrap adjust for this?

E.g. have an platform.compile(componentType, binding):Promise<CompiledApplication> and a CompileApplication.instantiate():ComponentRef and a
platform.compileAndCreate(componentType, bindings):Promise<ComponentRef>.

Copy link
Copy Markdown
Contributor

@tbosch tbosch Aug 27, 2015

Choose a reason for hiding this comment

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

This separation is important for serverside rendering, as there we compile once, cache the result, and create the root component multiple times (on every request).

@alxhub
Copy link
Copy Markdown
Member Author

alxhub commented Aug 27, 2015

I've solved the problem I was having with bootstrap() backwards compatibility: bootstrap() accepts the user's bindings for the application, but can't determine which bindings belong in the application injector and which ones belong in the component. Some bindings, like Router, need to exist in the component injector because they rely on the current component, while others such as an application API service should be shared among all of an app's root components.

A solution to this problem is to introduce Platform.simpleApplication(type, bindings), which returns a Promise<RootComponentRef> and uses a single injector but only supports one root component. bootstrap() then accepts any bindings and doesn't run into the same scoping problem.

@alxhub alxhub force-pushed the bootstrapping branch 3 times, most recently from bcca6d7 to 5568762 Compare August 28, 2015 21:53
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Aug 31, 2015

@alxhub Awesome. Do you have some work in progress which you can share, so that we can start the review process?

Let's resolve these questions as part of the review process.

@alxhub
Copy link
Copy Markdown
Member Author

alxhub commented Sep 1, 2015

@mhevery This PR is the full bootstrapping change that's ready for review. I'm still having some issues with the Dart tests but I can work through those separately.

@mhevery mhevery assigned alxhub and unassigned mhevery Sep 1, 2015
@alxhub alxhub force-pushed the bootstrapping branch 10 times, most recently from e2f31f8 to 65f8aff Compare September 11, 2015 21:58
@alxhub alxhub force-pushed the bootstrapping branch 5 times, most recently from d1fa8ea to 6598feb Compare September 14, 2015 22:25
@alxhub
feat(core): Support multiple ChangeDetectors in a single LifeCycle.
b271bb6 
This allows a single LifeCycle to be shared among multiple root components, since each root component has its own ChangeDetector configured.
@alxhub
refactor(router): Switch from using APP_COMPONENT binding to a manual…
836720d 
… ROUTER_PRIMARY_COMPONENT binding.

With the coming bootstrapping changes, a single application (and thus Router) can have multiple root components. One of these needs to be identified as the "primary" component from which the Router will load its configuration. This is now done by providing a ROUTER_PRIMARY_COMPONENT binding to the primary component type.
@alxhub
feat(core): Add a long-form syntax for Angular bootstrapping.
c3b6d42 
This change adds a syntax for bootstrapping Angular on a page that allows more fine-grained control of the hierarchy created. platform() creates a platform injector (of which there can only be one). From the platform, .application() creates an Angular application including a Zone and all specified application bindings (e.g. for the DOM, HTTP, Compiler, Renderer, etc). At the application level, .bootstrap() will bootstrap the given component into that application.
@alxhub alxhub added the action: merge The PR is ready for merge by the caretaker label Sep 14, 2015
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Sep 14, 2015

User @alxhub does not have PR merging privileges.

@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Sep 14, 2015
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 14, 2015
@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Sep 14, 2015
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Sep 14, 2015

User @alxhub does not have PR merging privileges.

@alxhub
Copy link
Copy Markdown
Member Author

alxhub commented Sep 14, 2015

@mhevery: Looks like I can't add the label, but it should be good to merge. Feel free to add it if you think the PR is ready to go in.

@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Sep 14, 2015
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Sep 14, 2015

Merging PR #3852 on behalf of @mhevery to branch presubmit-mhevery-pr-3852.

@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Sep 14, 2015
@alxhub alxhub closed this in 97d1844 Sep 15, 2015
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 7, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@alxhub alxhub

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alxhub @mhevery @mary-poppins @tbosch @googlebot
X Tutup