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

feat(core): add DoBootstrap interface. #24558

Closed
wants to merge 1 commit into
base: master
from

Conversation

@RoopeHakulinen
Contributor

RoopeHakulinen commented Jun 18, 2018

Closes #24557.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #24557

What is the new behavior?

The DoBootstrap interface is added.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@NgModule({
imports: [BrowserModule],
declarations: [TestComponent],
entryComponents: [TestComponent],
})
class TestModule {
class TestModule implements DoBootstrap {

This comment has been minimized.

@mhevery

mhevery Jul 2, 2018

Member

That is not a best example since ngDoBootstrap does nothing. Could we create an example where the ngDoBoostrap would actually bootstrap the application.

This comment has been minimized.

@RoopeHakulinen

RoopeHakulinen Jul 2, 2018

Contributor

Thanks for the comment @mhevery! I totally agree but unfortunately I went through all of the current usages of ngDoBoostrap in the Angular source and none of them did anything else than what is needed in hybrid apps (prevented the bootstrapping, that is).

What would an ideal example look like in your opinion and where should I put it as there's no test case for it currently? Should some test case be added? If so, what would be a meaningful test case?

Is this the simplest way to use it?

class AppModule implements DoBootstrap {
  ngDoBootstrap(appRef: ApplicationRef) {
    appRef.bootstrap(AppComponent); // Or some other component
  }
}

This comment has been minimized.

@JoostK

JoostK Jul 2, 2018

Contributor

@RoopeHakulinen One use case for ngDoBootstrap is with @angular/elements

This comment has been minimized.

@RoopeHakulinen

RoopeHakulinen Jul 2, 2018

Contributor

@JoostK Thanks for bringing that up! Am I right, though, stating that in case of Angular Elements, the ngDoBootstrap will still have an empty body?

This comment has been minimized.

@mhevery

mhevery Jul 2, 2018

Member

In case of Elements it will do nothing, but in most cases the example you gave above is correct. We simply boostrap the app. The reason why this example is important is to show to the user how they can do work before or after the bootstrap.

This comment has been minimized.

@trotyl

trotyl Jul 3, 2018

Contributor

Possibly a dynamic element bootstrap example for #15668? Like:

class AppModule implements DoBootstrap {
  constructor(private cfr: ComponentFactoryResolver) { }

  ngDoBootstrap(appRef: ApplicationRef) {
    const componentFactory = this.cfr.resolveComponentFactory(AppComponent);
    appRef.bootstrap(componentFactory, document.getElementById('dynamic-container'));
  }
}

This comment has been minimized.

@JoostK

JoostK Jul 3, 2018

Contributor

@RoopeHakulinen I typically do the createCustomElement/customElements.define dance in ngDoBootstrap. Perhaps @robwormald can shed some light on the preferred location for registering your custom elements, I personally dislike doing it from the constructor.

@@ -274,3 +275,19 @@ function preR3NgModuleCompile(moduleType: InjectorType<any>, metadata: NgModule)
export const NgModule: NgModuleDecorator = makeDecorator(
'NgModule', (ngModule: NgModule) => ngModule, undefined, undefined,
(type: Type<any>, meta: NgModule) => (R3_COMPILE_NGMODULE || preR3NgModuleCompile)(type, meta));
/**
* @usageNotes

This comment has been minimized.

@mhevery

mhevery Jul 2, 2018

Member

The first line the docs should be a single sentence describing the purpose of this. Something like: `Lifecycle hook that is called during application bootstrapping process.

This comment has been minimized.

@RoopeHakulinen

RoopeHakulinen Jul 2, 2018

Contributor

Oh, copied it from somewhere and edited it improperly. Fixed now.

@mhevery mhevery self-assigned this Jul 2, 2018

@mhevery

This comment has been minimized.

Member

mhevery commented Jul 17, 2018

Any progress on this PR? I would love to merge this in, but I need the comments addressed.

@RoopeHakulinen

This comment has been minimized.

Contributor

RoopeHakulinen commented Jul 17, 2018

Thanks for pinging me @mhevery It is very much on my todo list but I've been busy unfortunately. Hoping to get back to it on weekend. I'm also still wondering where to put the code examples (as there is none in current code base) and which one would be the best one of different types (Angular elements, bootstrapping hybrid app or adding side effects before/after the bootstrapping)?

@mhevery

This comment has been minimized.

Member

mhevery commented Jul 25, 2018

Ping, I would love to get this merged...

@trotyl

This comment has been minimized.

Contributor

trotyl commented Jul 25, 2018

@mhevery I think you haven't make a conclusion what the example should be..

@mhevery

This comment has been minimized.

Member

mhevery commented Jul 27, 2018

Sorry, I thought we will be implementing @RoopeHakulinen comment:

class AppModule implements DoBootstrap {
  ngDoBootstrap(appRef: ApplicationRef) {
    appRef.bootstrap(AppComponent); // Or some other component
  }
}

We just need an example showing the happy path of bootstrapping the application programmatically rather than declaratively.

@RoopeHakulinen

This comment has been minimized.

Contributor

RoopeHakulinen commented Jul 28, 2018

@mhevery Updated it now to contain an inline example. :)

@RoopeHakulinen

This comment has been minimized.

Contributor

RoopeHakulinen commented Aug 7, 2018

Ping @mhevery :)

@mhevery

mhevery approved these changes Aug 7, 2018

@kara

This comment has been minimized.

Contributor

kara commented Aug 7, 2018

@kara kara closed this in 732026c Aug 7, 2018

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