Skip to content
This repository has been archived by the owner on Oct 29, 2023. It is now read-only.

Remove @nguniversal/* projects #388

Merged
merged 1 commit into from Oct 18, 2019
Merged

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Oct 15, 2019

Previously, the @nguniversal/* packages were imported into standard @angular/cli apps. However, these packages are not supposed to be used in Angular apps (on the client) and thus do not need to be processed by ngcc. They contain code that must be run on the server (with access to Node.js APIs) and thus the corresponding projects were failing during build, due to missing built-in Node.js modules (e.g. fs).

We should definitely test how ngUniversal apps work with Ivy (and ngcc), but this is not the right way. Removing the packages for now.

EDIT:
There are tests for ngUniversal+Ivy in the ngUniversal repo and in the @angular/cli repo. So, we seem to be covered 馃尰

This sits on top of #385. Only the last commit is new.

@gkalpak gkalpak mentioned this pull request Oct 15, 2019
45 tasks
chromeOptions: {
args: [ "--headless", "--disable-gpu"]
}
'browserName': 'chrome'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are from #385 (only the last commit is relevant for this PR).

Copy link
Contributor

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

Can you remove @nguniversal related deps from package.json?

@@ -32,11 +32,7 @@ jobs:
- run:
name: Install Dependencies
command: |
yarn install --frozen-lockfile
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this.

@mgechev
Copy link
Member

mgechev commented Oct 15, 2019

@gkalpak would you rebase?

@gkalpak
Copy link
Member Author

gkalpak commented Oct 16, 2019

@mgechev: Done.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

鈩癸笍 Googlers: Go here for more info.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

鈩癸笍 Googlers: Go here for more info.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

鈩癸笍 Googlers: Go here for more info.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

鈩癸笍 Googlers: Go here for more info.

Previously, the `@nguniversal/*` packages were imported into standard
`@angular/cli` apps. However, these packages are not supposed to be used
in Angular apps (on the client) and thus do not need to be processed by
`ngcc`. They contain code that must be run on the server (with access to
Node.js APIs) and thus the corresponding projects were failing during
build, due to missing built-in Node.js modules (e.g. `fs`).

We should definitely test how ngUniversal apps work with Ivy (and
`ngcc`), but this is not the right way. Removing the packages for now.
@googlebot
Copy link

CLAs look good, thanks!

鈩癸笍 Googlers: Go here for more info.

@gkalpak
Copy link
Member Author

gkalpak commented Oct 18, 2019

FYI: Rebased on master.

@mgechev mgechev merged commit f048c25 into angular:master Oct 18, 2019
@gkalpak gkalpak deleted the remove-nguniversal branch October 19, 2019 07:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants