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

feat(core): integrated bootstrap #483

Closed

Conversation

dsebastien
Copy link
Contributor

Closes #112 #412

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

return modRef;
decorateModule(moduleRef: NgModuleRef<any>): NgModuleRef<any> {
// perform any module customization needed for this specific environment here
// and make sure to invoke this function by passing it the NgModule created by Angular
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the formatting is not correct...

/* do something here */
decorateModule(moduleRef: NgModuleRef<any>): NgModuleRef<any> {
// perform any module customization needed for this specific environment here
// and make sure to invoke this function by passing it the NgModule created by Angular
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the formatting is not correct...

decorateModule(moduleRef: NgModuleRef<any>): NgModuleRef<any> {
// perform any module customization needed for this specific environment here
// and make sure to invoke this function by passing it the NgModule created by Angular
return moduleRef;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the formatting is not correct...

case "interactive":
case "complete":
default:
if (this.environment.hmr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we can extract this entire block to a function because it is also used in the mainWrapper property, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed

document.removeEventListener("DOMContentLoaded", this.mainWrapper, false);

if (this.environment.hmr) {
if (module["hot"]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be changed to module.hot to keep consistency since we use it like that in the other places in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mh for module.hot we need to look at this task first I think: #482

// no need for the event listener after this
document.removeEventListener("DOMContentLoaded", this.mainWrapper, false);

if (this.environment.hmr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if it is better to change this check to use the ambient ENV variable. This way the code inside will be removed in PROD. But in order for this to works, then we should also add this check inside the bootstrapHmr method so that the actual usage of the angularclass/hmr module is also removed from the bundle.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, let's keep it light :)

* Reference: https://github.com/gdi2290/angular-hmr
* @ignore
*/
protected bootstrapHmr: any = (module: any, bootstrap: () => Promise<NgModuleRef<any>>) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can change the type of this to Function ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

@dsebastien
Copy link
Contributor Author

I'll try to look at this soon, not sure when I'll be able to though :)

@dsebastien dsebastien force-pushed the feature/bootstrap branch from 4e70125 to 83f41e9 Compare July 5, 2018 05:43
@dsebastien dsebastien force-pushed the feature/bootstrap branch from 83f41e9 to d8151c8 Compare July 5, 2018 05:55
@dsebastien dsebastien closed this Jul 6, 2018
@dsebastien dsebastien deleted the feature/bootstrap branch July 6, 2018 08:28
@dsebastien
Copy link
Contributor Author

PR recreated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core: bootstrap
2 participants