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

fix(common): add serverTarget for prerender #2123

Merged
merged 1 commit into from
May 23, 2021
Merged

fix(common): add serverTarget for prerender #2123

merged 1 commit into from
May 23, 2021

Conversation

bampakoa
Copy link
Contributor

Add serverTarget for the prerender architect in the add schematic

Closes #2120

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution

Can you please format the code?

  yarn ng-dev format changed 

@alan-agius4 alan-agius4 added action: cleanup target: patch This PR is targeted for the next patch release labels May 23, 2021
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Actually, this is not the correct schematic, the problematic one is https://github.com/angular/universal/blob/master/modules/express-engine/schematics/install/index.ts

The changes here should be reverted

Add `serverTarget` for the prerender architect in the install schematic

Closes #2120
@@ -118,9 +118,11 @@ function updateWorkspaceConfigRule(options: AddUniversalOptions): Rule {
configurations: {
production: {
browserTarget: `${options.project}:build:production`,
serverTarget: `${projectName}:server:production`,
},
development: {
browserTarget: `${options.project}:build:development`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mind changing this to projectName? 😀

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM

@alan-agius4 alan-agius4 added action: merge PR author is ready for this to merge and removed action: cleanup labels May 23, 2021
@alan-agius4 alan-agius4 merged commit ccc0fc1 into angular:master May 23, 2021
@bampakoa
Copy link
Contributor Author

@alan-agius4 you mean to change projectName to options.project right?

@alan-agius4
Copy link
Collaborator

Other way.

@bampakoa
Copy link
Contributor Author

My browser was idle and I did not notice that the PR was actually merged 😁 Should I make a separate PR for the change that you propose?

@alan-agius4
Copy link
Collaborator

Sure, feel free to do a refactor PR

@bampakoa bampakoa deleted the add-schematic branch May 23, 2021 19:03
@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 Jun 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prerender fails on new angular-cli 12 app
3 participants