feat(common): introduce StateTransferInitializerModule by Toxicable · Pull Request #916 · angular/universal

Toxicable · 2018-03-07T07:01:30Z

Please check if the PR fulfills these requirements

- [x] The commit message follows
- [ ] Tests for the changes have been added (for bug fixes / features)
- [x] Docs have been added / updated (for bug fixes / features)

What modules are related to this pull-request

- [x] common

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Adds a new module to help with StateTransfer
What is the current behavior? (You can also link to an open issue here)
n/a
What is the new behavior (if this is a feature change)?
Adds a new module to delay the app starting, therefore delay statetransfer accessing the dom till it's ready

CaerusKaru · 2018-03-07T07:03:29Z

@@ -0,0 +1,13 @@
+# State Transfer Initializer
+Delays the app bootstrap proces to ensure that the DOM content is loaded before


/s/proces/process

Also the thought trails off, maybe "before state transfer initialization"

CaerusKaru · 2018-03-07T07:03:45Z

+# State Transfer Initializer
+Delays the app bootstrap proces to ensure that the DOM content is loaded before
+
+Simply import the module into your project and you will not longer need to wrap your component bootsrap function in an `DOMContentLoaded` callback


/s/not/no
/s/bootsrap/bootstrap
/s/an/a

CaerusKaru · 2018-03-07T07:04:53Z

+import { NgModule } from '@angular/core';
+
+export function domContentLoadedFactory(doc: Document) {
+  return () => {


Can't this function take a Promise directly, ie cut out the inner return?

Pretty sure it takes a fn which returns a Promise.
Will double check

Here's an example https://github.com/angular/angular/blob/c8a1a14b87e5907458e8e87021e47f9796cb3257/packages/platform-webworker/src/web_workers/worker/location_providers.ts#L35

That example has the signature:

() => Promise<>

So here it would be:

return () => new Promise((resolve, reject) => {});

That's what im doing

I'm saying get rid of the first pair of curly braces

I mean, it dosen't change anything but sure

CaerusKaru · 2018-03-07T07:05:40Z

+    return new Promise ((resolve, reject) => {
+      const contentLoaded = () => {
+        resolve();
+        doc.removeEventListener('DOMContentLoaded', contentLoaded);


Should this come before the resolve?

CaerusKaru

LGTM

CaerusKaru · 2018-03-07T07:35:41Z

+
+@NgModule({
+  providers: [
+    {provide: APP_INITIALIZER, multi: true, useFactory: domContentLoadedFactory, deps: [DOCUMENT]},


Missed this:

Missing import for DOCUMENT

CaerusKaru · 2018-03-07T07:37:01Z

+    {provide: APP_INITIALIZER, multi: true, useFactory: domContentLoadedFactory, deps: [DOCUMENT]},
+  ]
+})
+export class StateTransferInitializerModule


Also missing curly braces here

needs testing yo

I figured we'd add testing in a follow-up PR once we iron out our testing scheme

yupyup that was my plan aswell

I mean silly thiongs like that would be picked up there

CaerusKaru · 2018-03-07T10:29:33Z

+      resolve();
+    };
+    doc.addEventListener('DOMContentLoaded', contentLoaded);
+  };


Missing closing parens

angular-automatic-lock-bot · 2019-09-06T00:32:17Z

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.}

Toxicable requested review from CaerusKaru and vikerman as code owners March 7, 2018 07:01

googlebot added the cla: yes label Mar 7, 2018

CaerusKaru reviewed Mar 7, 2018

View reviewed changes

CaerusKaru added the action: review label Mar 7, 2018

Toxicable changed the title ~~feat(common): introduce StateTransferInitializerModule~~ [WIP]feat(common): introduce StateTransferInitializerModule Mar 7, 2018

CaerusKaru added state: WIP and removed action: review labels Mar 7, 2018

CaerusKaru approved these changes Mar 7, 2018

View reviewed changes

CaerusKaru reviewed Mar 7, 2018

View reviewed changes

Toxicable added the target: minor target: minor This PR is targeted for the next minor release label Mar 7, 2018

CaerusKaru reviewed Mar 7, 2018

View reviewed changes

CaerusKaru added this to the 6.0 milestone Mar 7, 2018

CaerusKaru reviewed Mar 7, 2018

View reviewed changes

Toxicable force-pushed the state-transfer-init branch from 6696d31 to 50d9ac1 Compare March 7, 2018 17:13

Toxicable changed the title ~~[WIP]feat(common): introduce StateTransferInitializerModule~~ feat(common): introduce StateTransferInitializerModule Mar 7, 2018

Toxicable added action: review and removed state: WIP labels Mar 7, 2018

feat(common): introduce StateTransferInitializerModule

55d159a

Toxicable force-pushed the state-transfer-init branch from 50d9ac1 to 55d159a Compare March 10, 2018 03:43

MarkPieszak approved these changes Mar 10, 2018

View reviewed changes

Toxicable merged commit 5aef476 into angular:master Mar 10, 2018

Toxicable deleted the state-transfer-init branch March 10, 2018 23:13

Toxicable pushed a commit that referenced this pull request Mar 18, 2018

feat(common): introduce StateTransferInitializerModule (#916)

77fe0fe

ArikEpstein mentioned this pull request Mar 21, 2018

introduce StateTransferInitializerModule - doesn't include to 5.0.0-beta.7 release #936

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 6, 2019

		@@ -0,0 +1,13 @@
		# State Transfer Initializer
		Delays the app bootstrap proces to ensure that the DOM content is loaded before

Conversation

Toxicable commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaerusKaru Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaerusKaru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot Bot commented Sep 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CaerusKaru Mar 7, 2018 •

edited

Loading