-
Notifications
You must be signed in to change notification settings - Fork 26.2k
docs(aio): update universal for CLI #20039
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks great, just a few minor nits to keep everything up to date in changes with the universal-starter and other v5 improvements
constructor( | ||
private http: HttpClient, | ||
@Optional() @Inject(APP_BASE_HREF) origin: string) { | ||
this.heroesUrl = (origin || '') + this.heroesUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.heroesUrl = `${origin}${this.heroesUrl}`
to maintain consistency with other methods in this file
|
||
// #docregion renderModuleFactory | ||
app.engine('html', (_, options, callback) => { | ||
renderModuleFactory(AppServerModuleNgFactory, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're still avoiding ngExpressEngine
here?
@NgModule({ | ||
imports: [ | ||
AppModule, | ||
ServerModule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also want to add the TransferState modules? Or is this update intended for both v4.4 and v5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This update will probably go for v5
@Optional() @Inject(APP_BASE_HREF) origin: string) { | ||
this.heroesUrl = (origin || '') + this.heroesUrl; | ||
} | ||
this.heroesUrl = (origin || '') + this.heroesUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same new string infix as before
|
||
module.exports = { | ||
entry: { server: "./server.ts" }, | ||
resolve: { extensions: [".ts", ".js"] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverse these to satisfy issues documented here: angular/preboot#41 (comment)
new webpack.ContextReplacementPlugin( | ||
/(.+)?express(\\|\/)(.+)?/, | ||
path.join(__dirname, "src") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing trailing {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where? I got an error there that disappeared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aio/content/guide/universal.md
Outdated
1. Show the first page quickly | ||
|
||
{@a seo} | ||
{@a web-crawlers} | ||
#### Facilitate web crawlers | ||
|
||
Google, Bing, Facebook, Twitter and other social media sites rely on web crawlers to index your application content and make that content searchable on the web. | ||
Google, Bing, Facebook, Twitter and other social media sites rely on web crawlers to index your application content and make that content searchable on the web. | ||
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/s/easy/easily
* `express` - node web server. | ||
* `@types/express` - TypeScript type definitions for express. | ||
* `@nguniversal/module-map-ngfactory-loader` - For handling lazy-loading in the context of a server-render. | ||
* `@nguniversal/express-engine` - An express engine for Universal applications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why install this if it's not mentioned anywhere else in this guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right with this and the previous comment. I mixed my snippets. It should use the engine.
</code-example> | ||
|
||
#### Serve static files safely | ||
|
||
A single `server.use()` treats all other URLs as requests for static assets | ||
A single `app.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) | ||
and will only honor requests for files from the `/dist` folder. | ||
|
||
The following express code routes all remaining requests to `/dist`; it returns a `404 - NOT FOUND` if the file is not found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? With the new server.ts
, I think it will just default back to the Angular router.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I haven't touched this, I will check if for a non existing static it goes to 404 or not.
@@ -613,8 +440,10 @@ First add the _build_ and _serve_ commands to the `scripts` section of the `pack | |||
<code-example format="." language="ts"> | |||
"scripts": { | |||
... | |||
"build:uni": "webpack --config webpack.config.universal.js", | |||
"serve:uni": "node dist/server.js", | |||
"build:universal": "npm run build:client-and-server-bundles && npm run webpack:server", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be build:ssr
to match the universal-starter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or the universal should match this 😂
I don't mind either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and still better than the :dynamic the CLI wiki has 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those targets have been changed as of angular/angular-cli#8256
e2e2a06
to
862ed71
Compare
0070c09
to
38f801b
Compare
This is fine and necessary for v5 release. We can circle back to polish in a future PR |
Installing dependencies for the docs examples fails, because the lockfile is out-of-sync with the corresponding `package.json`. This commit brings the lockfile in sync with `package.json`. (For reference, this was accidentally broken in angular#20039.)
Installing dependencies for the docs examples fails, because the lockfile is out-of-sync with the corresponding `package.json`. This commit brings the lockfile in sync with `package.json`. (For reference, this was accidentally broken in #20039.)
|
||
normalizePath(filePath) { | ||
// transform for example ../cli/src/tsconfig.app.json to src/tsconfig.app.json | ||
return filePath.replace(/\.{2}\/\w+\//, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\w
will not match -
. It may not matter for now, but generally it is better to use \/[^/]+\/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I am very bad constructing regexp and I made one that works for all our current use cases.
"@angular/compiler-cli", | ||
"@angular/platform-server", | ||
"express" | ||
"web-animations-js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not aplphabetically ordered 😞
// Temporary Fix for issue: https://github.com/angular/angular/issues/11580 | ||
// for 'WARNING Critical dependency: the request of a dependency is an expression' | ||
new webpack.ContextReplacementPlugin( | ||
/(.+)?angular(\\|\/)core(.+)?/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(.+)?
=== .*
But if you don't need the matched text, you can drop them altogether. They don't change what the RegExp matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied that straight from the CLI docs, haven't touch the file. Now that you mention it, I am using it differently in a starter of mine
|
||
getHeroes(): void { | ||
this.heroService.getHeroes() | ||
.subscribe(heroes => this.heroes = heroes.slice(1, 5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad indentation 😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I copied the toh-pt6 example, so I need to fix that there.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Part of #19510