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

docs: update documentation for universal #7796

Closed
wants to merge 9 commits into from
Closed

docs: update documentation for universal #7796

wants to merge 9 commits into from

Conversation

MarkPieszak
Copy link
Member

Closes #7706
Closes #7248

Can close PR #7739

cc/ @hansl @vikerman @alxhub @Toxicable

// * NOTE :: leave this as require() since this file is built Dynamically from webpack
const { AppServerModuleNgFactory, LAZY_MODULE_MAP } = require('./dist/server/main.bundle');

const { provideModuleMap } = require('@nguniversal/module-map-ngfactory-loader');

Choose a reason for hiding this comment

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

should include installation of this package in the installation section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


// Import renderModuleFactory from @angular/platform-server.
var renderModuleFactory = require('@angular/platform-server').renderModuleFactory;
// ALl regular routes use the Universal engine

Choose a reason for hiding this comment

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

ALl typo

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

var renderModuleFactory = require('@angular/platform-server').renderModuleFactory;
// ALl regular routes use the Universal engine
app.get('*', (req, res) => {
res.render('index', { req });

Choose a reason for hiding this comment

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

This assumes you're running the server.js in the same folder as your index.html yet below we describe using dist/browser, maybe sync those up?

Copy link
Member Author

Choose a reason for hiding this comment

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

You don't need directory stuff here, we are just calling the engine here

Choose a reason for hiding this comment

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

I meant the .render('index' dosen't that look for index.html in the directory it's being executed in?

Copy link
Member Author

Choose a reason for hiding this comment

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

It has reference to it, since it's a lookup of the View, so having just index alone works since it knows which oneto find. You can be explicit and do res.render(join(DIST_FOLDER, 'browser', 'index.html'), { req }); but there's no need. ExpressEngines are almost always just a string like this one.

});
```

## Step 5: Setup a webpack config to handle this Node server.ts file and serve your application!

Choose a reason for hiding this comment

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

Mention that this is work around for #7200 ?
It makes sense for the dynamic case since you shouldn't have to do a npm install on your server, but for static not so much (if #7200 wasn't an issue)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'll put that in there, good idea! Forgot the issue number 👍

@hansl
Copy link
Contributor

hansl commented Sep 23, 2017

You will want to remove the universal part of your commits (put it in the title please). So basically something like docs: update documentation for universal. And it seems like 3 commits are a bit much; is it really 3 pieces of work?

Aside from the cosmetic part, I'll let the Universal team review this PR.

@CaerusKaru
Copy link
Member

@MarkPieszak Can you provide at least some notes about the webpack warnings that arise? Perhaps mention the partial solution given by @gdi2290 at angular/angular#11580 (comment)

E.g.

WARNING in ./node_modules/@angular/core/@angular/core.es5.js
5675:15-102 Critical dependency: the request of a dependency is an expression

It may warrant further discussion since these issues crop up with a lot of express dependencies, e.g. express itself, shrink-wrap, etc

@MarkPieszak MarkPieszak changed the title docs(universal-rendering): update to use typescript and webpack docs: update documentation for universal Sep 23, 2017
@MarkPieszak
Copy link
Member Author

Thanks @hans, will make some updates & squash the commits. 👍
@CaerusKaru Added a note to it and included it in the webpack.server.config.js file as well, to avoid the issue for people completely (i'll update the universal-starter / cli with this as well) :)

docs(universal): updates

remove duplicate universal-starter link

pick pocs: update with changes requested
docs(universal): updates

remove duplicate universal-starter link

pick pocs: update with changes requested

docs(universal): updates

remove duplicate universal-starter link

docs: update with changes requested

update views dist_folder
@MarkPieszak
Copy link
Member Author

Closing for PR #7803 as history got out of control on this one.

@MarkPieszak MarkPieszak deleted the universal-readme branch September 23, 2017 20:32
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants