docs: update universal guide based on PR 17573 #18707

Revises both universal and client build to use AOT and webpack for both. Guide text adjusted accordingly Dodges CLI client build, expected in near future.

mary-poppins · 2017-10-06T22:54:59Z

You can preview 346c676 at https://pr18707-346c676.ngbuilds.io/.

mary-poppins · 2017-10-06T23:21:53Z

You can preview 4847bc3 at https://pr18707-4847bc3.ngbuilds.io/.

CaerusKaru

I think it's a big mistake to not include out-of-the-box support for Angular CLI and ngExpressEngine. Not including these widely-used tools may lead to user confusion and bad practice out of the gate. Many of the guides use these tools and they make for a clearer process. I don't think it would require a lot of refactoring to get this guide in line with those tools, and I would volunteer to do the work myself if it's too much, @wardbell.

I understand the desire to get this guide out ASAP, but I think it's best to hold off, since there are other guides out there in the interim, if it means getting the best guide possible.

All in all though, excellent work putting this together @wardbell, and I don't mean to be harsh.

CaerusKaru · 2017-10-07T17:16:12Z

aio/content/examples/universal/src/universal/server.ts

+
+// #docregion navigation-request
+// simplistic regex matches any path without a '.'
+const pathWithNoExt = /^([^.]*)$/;


This is unnecessary, you can just have an app.use('*.*', express.static('dist')); for the static files, and then

app.get('*', (req, res) => { res.render('index-universal.html', { req }); }

CaerusKaru · 2017-10-07T17:19:45Z

aio/content/guide/universal.md

+
+These web crawlers may be unable to navigate and index your highly-interactive, Angular application as a human user could do.
+
+Angular Universal can generate a static version of your app that is easy searchable, linkable, and navigable without JavaScript.


/s/easy/easily

CaerusKaru · 2017-10-07T17:24:32Z

aio/content/guide/universal.md

+
+You must make a few changes to your application code to support both server-side rendering and the transition to the client app.
+
+#### The root `AppModule`


You should add a comment on preventing screen "flicker" by adding initialNavigation: 'enabled' to a user's root RouterModule

CaerusKaru · 2017-10-07T17:33:45Z

aio/content/guide/universal.md

+
+This guide's sample is written for [Node Express](https://expressjs.com/)
+so the engine takes the form of [Express template engine middleware](https://expressjs.com/en/guide/using-template-engines.html).
+


authentication and unauthorized access to the rendered pages.

The renderModuleFactory doesn't handle this either. It's a function of the client app to handle this browser-side, i.e. with route guards, and for the /api handle to implement authentication/authorization for those guards with a token/cookie

differentiating static file requests from navigation requests

This is also not handled by the renderModuleFactory directly, it's a function of where you specify server.get('*.*', express.static('dist')), i.e. before the rendering process

where the client sends requests for data.

This is also not handled by the renderModuleFactory, it's a function of where you specify server.get('api/*', ...), i.e. before the rendering process

Do we really want the server to (optionally) compile the module on the server? Shouldn't it be precompiled in the build process as the current guide recommends?

Using the CLI, the module is precompiled, and then used by the ngExpressEngine. You can also pass in the compiled module from your build process. They are equivalent. If you mean precompiling the routes, the universal-starter has a route prerendering element, linked below

It is not clear from the README how you would use this server. It doesn't look like a "normal" express server as most express developers understand "normal". The repo lacks an example.
I can't see the point of recommending two express servers.

ngExpressEngine is not a server, it is a rendering engine for use with server.engine('html', ...). In this way, it is almost entirely comparable with your method, except it is far cleaner

It's not like the ngExpressEngine is self-explanatory

This is a great reason to clarify its use in this guide

Finally, it takes a different approach than the guide and we'd have to alter the proposed guide considerably to accommodate it

Again, since it is mostly comparable to renderModuleFactory, except clearer, it shouldn't be considerably difficult to add the changes

The repo lacks an example

The universal-starter that @Toxicable mentioned has a fantastic example of incorporating the ngExpressEngine, found here

CaerusKaru · 2017-10-07T17:37:40Z

aio/content/guide/universal.md

+In fact, if the app were _only_ rendered by the server, _every_ app link clicked would arrive at the server
+as a navigation URL intended for the router.
+
+Fortunately, application routes have something in common: their URLs lack file extensions.


See nit about this in the server file. Should use express.static instead of creating unnecessary filters and checks

CaerusKaru · 2017-10-07T17:42:45Z

aio/content/guide/universal.md

+A single `server.use()` treats all other URLs as requests for static assets
+such as JavaScript, image, and style files.
+
+To ensure that clients can only download the files that they are _permitted_ to see, you will [put all client-facing asset files in the `/dist` folder](#universal-webpack-configuration)


See nit above about using express.static

CaerusKaru · 2017-10-07T17:50:17Z

aio/content/examples/universal/src/app/hero-search.service.ts

+  constructor(
+    private http: HttpClient,
+    @Optional() @Inject(APP_BASE_HREF) origin: string) {
+    this.searchUrl = (origin || '') + this.searchUrl;


Maybe change to this.searchUrl = `${origin}${this.searchUrl}` to be in line stylistically with other examples, and the very definition of origin in getOrigin?

CaerusKaru · 2017-10-07T17:50:45Z

aio/content/examples/universal/src/app/hero.service.ts

+  constructor(
+    private http: HttpClient,
+    @Optional() @Inject(APP_BASE_HREF) origin: string) {
+    this.heroesUrl = (origin || '') + this.heroesUrl;


Same nit here as above

CaerusKaru · 2017-10-07T17:51:38Z

aio/content/examples/universal/src/app/hero.service.ts

-import 'rxjs/add/operator/toPromise';
-// #enddocregion rxjs
+import { Observable } from 'rxjs/Observable';
+import 'rxjs/add/operator/catch';


This is an RxJS anti-pattern. Please use .call on each operator, or wait until Angular supports RxJS 5.5.0 and its lettable operators

Explain? I am interested.

See angular/components#2622

@CaerusKaru This is an anti-pattern in a library because you risk screwing the consumer of your library, someone you do not and cannot know.

Patching the prototype is OK in your own application. You're not screwing anyone but yourself. The call syntax is bloody awful. It completely destroys the dev experience. Save it for libraries.

The library/app difference is a critically important distinction!

I do hope we all move to lettable when it releases. But it is still in beta as I write this.

Sadly there's a side effect of even using the lettable method of rxjs right now, it's that if you use that method along side the prototype patching method then you'll end up with duplicated operators in your bundle.
From memory Webpack will solve this in v4 but for now it kinda of stops any libs from using it.
Sorry I don't have a link for this, just recalling it from some issues I've read

I agree with the reasoning for libraries, but it's also a benefit for tree-shaking. By patching when you need it, you avoid patching everywhere.

I also agree that the call syntax is terrible, I wanted to leave this note here primarily for when lettable operators are usable in Angular core.

CaerusKaru · 2017-10-07T18:01:00Z

aio/content/guide/universal.md

+npm install webpack @ngtools/webpack copy-webpack-plugin raw-loader @types/express --save-dev
+</code-example>
+
+{@a transition}


Add a section on capturing user processes using @angular/preboot? This should probably be a side note, because getting preboot to work with the current example would require significant refactoring

I was asked to remove the reference to preboot that I had in an early version of this guide.

- based on original effort in PR 17573 PR Close angular#18707

Revises both universal and client build to use AOT and webpack for both. Guide text adjusted accordingly Dodges CLI client build, expected in near future. PR Close angular#18707

angular#18707) PR Close angular#18707

Revises both universal and client build to use AOT and webpack for both. Guide text adjusted accordingly Dodges CLI client build, expected in near future. PR Close #18707

#18707) PR Close #18707

wardbell · 2017-10-10T06:45:57Z

@CaerusKaru We discussed timing internally and decided to forge ahead and revisit as quickly as alternatives allow. It's a judgment call and not mine to make.

I'll look at your other comments when I break free. Thanks very much for making the effort to comment!

wardbell · 2017-10-10T06:57:15Z

aio/content/guide/universal.md

+When a browser makes an HTTP request, the server can make assumptions about cookies, XSRF headers, etc.
+
+For example, the browser automatically sends auth cookies for the current user.
+Angular Universal cannot forward these credentials to a separate data server.


I'm parroting what I believe I was told by @alxhub

It may be that I'm using ngExpressEngine, which injects the req and res objects from Express by default. Regardless, it's definitely possible to add this functionality in only a couple of lines of code.

Here's the snippet from ngExpressEngine:
https://github.com/angular/universal/blob/2a1246326d5db10a989f143205c2d2ed27f8e362/modules/express-engine/src/main.ts#L129-L143

chuckjaz · 2017-10-11T20:54:09Z

@wardbell This PR does not apply cleanly on 4.4.x. Can you create a 4.4.x PR with these changes?

wardbell · 2017-10-25T19:47:00Z

@chuckjaz I believe @vikerman is dealing with the 4.4 problems.

angular-automatic-lock-bot · 2019-09-12T19:20: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.}

wardbell added comp: docs state: WIP labels Aug 15, 2017

wardbell self-assigned this Aug 15, 2017

googlebot added the cla: yes label Aug 15, 2017

steveschmitt mentioned this pull request Aug 15, 2017

[WIP] docs: add example for Angular Universal #17573

Closed

3 tasks

wardbell changed the title ~~docs: update universal guide based on PR 17573~~ [WIP] docs: update universal guide based on PR 17573 Aug 15, 2017

wardbell force-pushed the docs-universal branch from 719a225 to b6f47cd Compare August 15, 2017 17:46

wardbell added target: major This PR is targeted for the next major release area: core Issues related to the framework runtime labels Aug 15, 2017

wardbell added this to WIP in docs Aug 15, 2017

wardbell force-pushed the docs-universal branch from b6f47cd to 4f5e768 Compare August 15, 2017 21:55

wardbell force-pushed the docs-universal branch from 6ed6562 to 6ee9475 Compare August 16, 2017 04:28

wardbell force-pushed the docs-universal branch 2 times, most recently from 28918f9 to e589de4 Compare August 16, 2017 07:42

wardbell force-pushed the docs-universal branch from e589de4 to bb2f1c2 Compare August 17, 2017 05:11

wardbell force-pushed the docs-universal branch 2 times, most recently from b8fb21f to ab75906 Compare August 18, 2017 09:09

wardbell force-pushed the docs-universal branch from ab75906 to 10f8178 Compare August 18, 2017 09:24

wardbell force-pushed the docs-universal branch from b0df0df to 6e38df2 Compare October 6, 2017 02:43

wardbell force-pushed the docs-universal branch from 6e38df2 to 8ecc585 Compare October 6, 2017 22:39

wardbell added 2 commits October 6, 2017 15:40

docs: add universal guide

0db4039

- based on original effort in PR 17573

docs: add universal guide with production client app

0862242

Revises both universal and client build to use AOT and webpack for both. Guide text adjusted accordingly Dodges CLI client build, expected in near future.

wardbell force-pushed the docs-universal branch from 8ecc585 to 346c676 Compare October 6, 2017 22:40

docs: add universal guide with production client app - with JK’s edits

4847bc3

wardbell force-pushed the docs-universal branch from 346c676 to 4847bc3 Compare October 6, 2017 23:12

wardbell added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews state: WIP labels Oct 6, 2017

CaerusKaru requested changes Oct 7, 2017

View reviewed changes

chuckjaz added this to the Merge-queue milestone Oct 9, 2017

chuckjaz pushed a commit to chuckjaz/angular that referenced this pull request Oct 9, 2017

docs: add universal guide (angular#18707)

f03eaa6

- based on original effort in PR 17573 PR Close angular#18707

chuckjaz pushed a commit to chuckjaz/angular that referenced this pull request Oct 9, 2017

docs: add universal guide with production client app - with JK’s edits (

4f4ca8a

angular#18707) PR Close angular#18707

chuckjaz closed this in 0b0d25f Oct 9, 2017

chuckjaz pushed a commit that referenced this pull request Oct 9, 2017

docs: add universal guide with production client app - with JK’s edits (

0ea5f8b

#18707) PR Close #18707

wardbell deleted the docs-universal branch October 10, 2017 06:42

wardbell commented Oct 10, 2017

View reviewed changes

m98 mentioned this pull request Dec 14, 2017

Please update the documentation with the newest NG5, Domino & Universal releases angular/universal#865

Closed

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


		These web crawlers may be unable to navigate and index your highly-interactive, Angular application as a human user could do.

		Angular Universal can generate a static version of your app that is easy searchable, linkable, and navigable without JavaScript.


		You must make a few changes to your application code to support both server-side rendering and the transition to the client app.

		#### The root `AppModule`


		This guide's sample is written for [Node Express](https://expressjs.com/)
		so the engine takes the form of [Express template engine middleware](https://expressjs.com/en/guide/using-template-engines.html).

docs: update universal guide based on PR 17573 #18707

docs: update universal guide based on PR 17573 #18707

Conversation

wardbell commented Aug 15, 2017 • edited

mary-poppins commented Aug 15, 2017

mary-poppins commented Aug 15, 2017

mary-poppins commented Aug 15, 2017

mary-poppins commented Aug 16, 2017

mary-poppins commented Aug 16, 2017

mary-poppins commented Aug 16, 2017

mary-poppins commented Aug 16, 2017

mary-poppins commented Aug 17, 2017

mary-poppins commented Aug 18, 2017

mary-poppins commented Aug 18, 2017

mary-poppins commented Aug 18, 2017

mary-poppins commented Oct 6, 2017

mary-poppins commented Oct 6, 2017

mary-poppins commented Oct 6, 2017

CaerusKaru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaerusKaru Oct 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardbell Oct 10, 2017 • edited

Choose a reason for hiding this comment

Toxicable Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardbell commented Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckjaz commented Oct 11, 2017

wardbell commented Oct 25, 2017

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

wardbell commented Aug 15, 2017 •

edited

CaerusKaru Oct 7, 2017 •

edited

wardbell Oct 10, 2017 •

edited

Toxicable Oct 10, 2017 •

edited

wardbell commented Oct 10, 2017 •

edited