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

build: update NX #3251

Merged
merged 8 commits into from
Dec 2, 2021
Merged

build: update NX #3251

merged 8 commits into from
Dec 2, 2021

Conversation

timdeschryver
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[x] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

While testing #3245, some tests of the example app weren't working anymore.
With this update, all tests are back to green.
To upgrade, I used the nx migrate commands.

nx migrate latest
nx migrate --run-migrations

@ngrxbot
Copy link
Collaborator

ngrxbot commented Nov 23, 2021

Preview docs changes for b165520 at https://previews.ngrx.io/pr3251-b1655200/

@timdeschryver
Copy link
Member Author

Yikes, I'll have to take a look at the failing steps. Seems like these are green locally.
I'm afraid that this will be for next week though.

@timdeschryver timdeschryver force-pushed the update-nx branch 3 times, most recently from dcb8028 to a19d660 Compare November 30, 2021 20:04
@timdeschryver timdeschryver force-pushed the update-nx branch 2 times, most recently from 3dc48d1 to 03ae744 Compare December 1, 2021 11:25
@@ -113,7 +113,7 @@ jobs:
# Test
- run:
name: Run All Unit Tests
command: yarn nx run-many --target=test --all
command: yarn nx run-many --target=test --all --parallel=1
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to add this to make make the circle ci happy.
Otherwise, the following errors were thrown randomly.

> nx run component:test 
Nx could not find process's output. Run the command without --parallel.

> nx run component-store:test 
Nx could not find process's output. Run the command without --parallel.

I was under the impression that this was handled by the migration by NX.
image

@brandonroberts this could maybe be a bug in NX?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, if it's set in the nx.json, it shouldn't be necessary to manually set it everywhere for CI

@@ -33,6 +33,11 @@
{
"files": ["*.ts"],
"rules": {
"@typescript-eslint/no-explicit-any": "off",
Copy link
Member Author

Choose a reason for hiding this comment

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

This was bloating the output so I disabled it 😅

@markostanimirovic markostanimirovic merged commit 49b888b into master Dec 2, 2021
@timdeschryver timdeschryver deleted the update-nx branch December 2, 2021 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants