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

chore: rework on Nx Cache and Doc #2338

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chore: rework on Nx Cache and Doc #2338

wants to merge 2 commits into from

Conversation

kpanot
Copy link
Contributor

@kpanot kpanot commented Oct 24, 2024

Proposed change

Rework on Nx Cache for clarification and simplification
Add documentation regarding Nx PAT new process

@kpanot kpanot requested a review from a team as a code owner October 24, 2024 07:20
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
nx.json Show resolved Hide resolved
When building (`yarn build`) the project on `main` (or other `release/*`) branch, the Remote Cache will be downloaded.

> [!IMPORTANT]
> This feature is available for `@amadeus.com` email addresses, Nx Cloud Account should be created with a `@amadeus.com` email or it should be set as **main email** on Github if the Github account is used to register to Nx Cloud.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a bit weird to document something this specific in this repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an actual restriction, I think it is better to indicate it rather than hiding it.
But I am open if you think we should hide it.

Copy link
Contributor

Choose a reason for hiding this comment

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

"if the Github account is used to register to Nx Cloud." I'm not sure I understand what that part means

Copy link
Contributor Author

@kpanot kpanot Oct 24, 2024

Choose a reason for hiding this comment

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

When you follow the link in the doc, you get

image

The message means : use the "Continue with Github" button to register to Nx Cloud

"@angular-devkit/schematics-cli": "^18.0.5",
"@angular/cli": "~18.2.0",
"@o3r/schematics": "workspace:^",
"@openapitools/openapi-generator-cli": "~2.14.0",
"@schematics/angular": "~18.2.0"
"@schematics/angular": "~18.2.0",
"openapi-types": "^12.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this for the nx refacto?

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is valid for all the package dependency addition / updates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained, these are missing optional peer dependencies detected by Nx since the refactor.
The fact that they have not been detected before is suspicious :S

@kpanot kpanot force-pushed the chore/nx-cache branch 3 times, most recently from e9f88f4 to 2a57064 Compare October 25, 2024 02:32
@kpanot kpanot force-pushed the chore/nx-cache branch 4 times, most recently from 6f898c8 to e081f58 Compare October 25, 2024 05:23
.github/workflows/nx.yml Outdated Show resolved Hide resolved
fpaul-1A
fpaul-1A previously approved these changes Oct 25, 2024
@@ -25,6 +25,7 @@
"@actions/github": "^6.0.0",
"@angular-devkit/build-angular": "~18.2.0",
"@angular-devkit/core": "~18.2.0",
"@angular-devkit/schematics": "~18.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it needed ?
It's a GH app without any deps on Angular.
Same remark for the others @angular-devkit/* and @angular/*
For @angular-eslint/* we can discuss (for the eslint migration) if we keep these rules or not when the package does not depends on @angular/core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are need from Nx point of view for the following reasons:

  • They are imported (even with import type)
  • They are part of exported file (even builders are exported)

import type dependencies should be provided in case the skipLibCheck: false (which enforce the provide of type dependencies) but as it is not runtime dependencies, they can be flagged as optional

"openapi-types": {
"optional": true
},
"type-fest": {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't type-fest used only for jest test and so be only in devDep ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment for this addition in other package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nop, it is used to exported Schematics and in CLIs

"rxjs": "^7.8.1",
"sass": "~1.80.0",
"semver": "^7.5.2"
"semver": "^7.5.2",
"typescript": "~5.5.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't typescript be only a devDep ?

Copy link
Contributor

Choose a reason for hiding this comment

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

same remark for other packages that have this addition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in exported builders :S

doc: add documentation regarding Nx PAT new process
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment