feat(common): add UniversalEngine to encapsulate rendering by CaerusKaru · Pull Request #912 · angular/universal

CaerusKaru · 2018-02-25T01:13:15Z

Abstract the common rendering patterns and caching into
a UniversalEngine
Update build parameters to reflect dependency on
common for existing engines

BREAKING CHANGE:

The ASP.NET Core, Express, and Hapi engines now have
the Common package as a peer dependency
The tokens for all engines are now exported from
@nguniversal/common

E.g.

BEFORE:
import {REQUEST, RESPONSE} from '@nguniversal/express-engine/tokens';

AFTER:
import {REQUEST, RESPONSE} from '@nguniversal/common';

CaerusKaru · 2018-02-25T01:13:44Z

Full credit to @Toxicable for getting this together. Removed from the commit history because the clabot is evil.

Toxicable

A couple of things that need to be applied to all engines

Toxicable · 2018-02-25T06:23:43Z

@@ -0,0 +1,12 @@
+// Configuration for IDEs only.


this was left over from the factor, should be removed

I think it's important to note that this file should not be extended, and is not being used to build the package

What's it being used for then?

IDEs, as noted by the comment. It's so that the imports are not considered invalid while editing files.

Toxicable · 2018-02-25T06:24:19Z

-    "baseUrl": "."
+    "baseUrl": ".",
+    "paths": {
+      "@nguniversal/common": ["../../dist/packages/common"]


Should point to ../common - the src not the output, unless this was the fix to the issue I had

This was exactly the fix to the issue you had

Toxicable · 2018-02-25T06:26:18Z

+/**
+ * ResourceLoader implimentation for loading files
+ */
 export class FileLoader implements ResourceLoader {


Should be @Interval along with the other ones under UniversalEngine

This file is not exported in the public api so is by default internal

Unless you mean I should just add an internal comment? In which case sure

Toxicable · 2018-02-25T06:27:28Z

+import {FileLoader} from './file-loader';
+import {RenderOptions} from './interfaces';
+
+export class UniversalEngine {


should be @internal

Same as above

Also, I don't think this should be internal. I think the whole point is to encourage usage, even outside of the nguniversal-scope engines.

That wasn't my original plan. I think for now we should keep it internal, we can always externalize it later on, we cannot internalize it once it's already external

Alright I've marked it and its methods internal for now, but we really should look at this soon to externalize

Toxicable · 2018-02-25T06:27:58Z

@@ -0,0 +1,9 @@
+// Configuration for IDEs only.


also unneeded

Same as above

Toxicable · 2018-02-25T06:28:49Z

@@ -0,0 +1,12 @@
+// Configuration for IDEs only.


Same as above

CaerusKaru · 2018-02-25T21:21:31Z

Setting to blocked while we decide when this should be merged.

Toxicable · 2018-04-06T20:29:06Z

 const libNames = [
-  'aspnetcore-engine',
  'common',
+  'aspnetcore-engine',


any reason for this?

Not anymore, was leftover from when we used the pre-Material build system. The rationale being that common needed to be built before everything else

Toxicable · 2018-04-06T20:30:18Z

 node_repositories(package_json = ["//:package.json"])


-local_repository(


This is easier to leave as is since it'll use the one that Angular core depends on, otherwise we could get out of sync

Point taken

Toxicable · 2018-04-06T20:32:51Z

 export { StateTransferInitializerModule } from './src/state-transfer-initilizer/module';
+export { UniversalEngine as ɵUniversalEngine } from './src/universal-engine/universal-engine';
+export { NgSetupOptions } from './src/universal-engine/interfaces';
+export { REQUEST, RESPONSE, ORIGIN_URL } from './src/tokens/index';


Tokens should be a secondary entry point here.
Otherwise we'll get size regressions

Toxicable · 2018-04-06T20:35:01Z

 */
-export * from './public_api';
+export * from './interfaces';
+export {UniversalEngine as ɵUniversalEngine} from './universal-engine';


could you make a private export file for common and do the renaming there

Toxicable

Just the secondary entry on tokens then should be good.
But blocking the merge for now till we do a release with our current status

Toxicable · 2018-04-06T21:44:59Z

        "*.ts",
        "src/**/*.ts",
    ]),
-    module_name = "@nguniversal/express-engine",


put this back in

Toxicable · 2018-04-08T10:07:46Z

There's a regression here with TransferHttpCacheModule
Since it's bundled into the same bundle as the Universal Engine, it causes the TransferHttpCacheModule import to bring domino and other server side symbols with it.

I propose we make a secondary entry for the Engine to avoid any more API changes

Toxicable · 2018-04-27T00:44:06Z

We should reexport the tokens from their old locations but depricate them so this isn't a breaking change, to be removed in v7

* Abstract the common rendering patterns and caching into a UniversalEngine * Update build parameters to reflect dependency on common for existing engines BREAKING CHANGE: * The ASP.NET Core, Express, and Hapi engines now have the Common package as a peer dependency * The tokens for all engines are now exported from `@nguniversal/common` E.g. BEFORE: `import {REQUEST, RESPONSE} from '@nguniversal/express-engine/tokens';` AFTER: `import {REQUEST, RESPONSE} from '@nguniversal/common';`

CaerusKaru · 2018-05-19T04:01:32Z

Closed in favor of #996

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

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

CaerusKaru added the action: review label Feb 25, 2018

googlebot added the cla: yes label Feb 25, 2018

CaerusKaru force-pushed the fabian/common branch from 23732d0 to 83d7bac Compare February 25, 2018 01:15

CaerusKaru assigned vikerman and Toxicable Feb 25, 2018

Toxicable suggested changes Feb 25, 2018

View reviewed changes

CaerusKaru force-pushed the fabian/common branch from 83d7bac to 4e2fe07 Compare February 25, 2018 06:43

Toxicable mentioned this pull request Feb 25, 2018

feat(common): introduce common engine #780

Closed

CaerusKaru force-pushed the fabian/common branch from 4e2fe07 to 0ddb84d Compare February 25, 2018 07:02

Toxicable approved these changes Feb 25, 2018

View reviewed changes

CaerusKaru added PR state: LGTM state: blocked and removed action: review labels Feb 25, 2018

CaerusKaru force-pushed the fabian/common branch from 0ddb84d to b8208c9 Compare March 1, 2018 16:22

CaerusKaru requested review from MarkPieszak and vikerman as code owners March 1, 2018 16:22

CaerusKaru force-pushed the fabian/common branch from b8208c9 to 514ed07 Compare March 1, 2018 16:25

Toxicable added this to the 6.0 milestone Mar 7, 2018

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

CaerusKaru force-pushed the fabian/common branch from 514ed07 to 9bddc48 Compare March 9, 2018 23:33

Toxicable added state: WIP and removed PR state: LGTM state: blocked labels Mar 10, 2018

Toxicable added the state: blocked label Mar 31, 2018

CaerusKaru force-pushed the fabian/common branch 4 times, most recently from ef0e48a to 5d50a23 Compare April 6, 2018 20:07

Toxicable reviewed Apr 6, 2018

View reviewed changes

CaerusKaru force-pushed the fabian/common branch from 5d50a23 to 60d633a Compare April 6, 2018 20:29

Toxicable reviewed Apr 6, 2018

View reviewed changes

CaerusKaru force-pushed the fabian/common branch from 60d633a to 26c71ed Compare April 6, 2018 20:32

Toxicable reviewed Apr 6, 2018

View reviewed changes

Toxicable suggested changes Apr 6, 2018

View reviewed changes

CaerusKaru force-pushed the fabian/common branch 3 times, most recently from 4c2a51d to 763bc2e Compare April 6, 2018 21:24

Toxicable reviewed Apr 6, 2018

View reviewed changes

CaerusKaru force-pushed the fabian/common branch from 763bc2e to 12b793b Compare April 6, 2018 21:57

Toxicable force-pushed the fabian/common branch from 12b793b to 77b7a24 Compare May 18, 2018 21:59

Toxicable mentioned this pull request May 18, 2018

feat(common): add CommonEngine to encapsulate rendering #996

Merged

CaerusKaru closed this May 19, 2018

CaerusKaru deleted the fabian/common branch May 19, 2018 04:01

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

		node_repositories(package_json = ["//:package.json"])


		local_repository(

Conversation

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

Uh oh!

Uh oh!

CaerusKaru commented Feb 25, 2018

Uh oh!

Toxicable 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!

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 commented Feb 25, 2018

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!

Toxicable left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toxicable commented Apr 8, 2018

Uh oh!

Toxicable commented Apr 27, 2018

Uh oh!

CaerusKaru commented May 19, 2018

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CaerusKaru commented Feb 25, 2018 •

edited

Loading