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(js): unique hash for each execution #14154

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

wSedlacek
Copy link
Contributor

@wSedlacek wSedlacek commented Jan 5, 2023

Context

A few weeks ago there was an attempt to make it so that you could invoke more
then one @nrwl/js:node per process. This change introduced a uniqueKey in
order to make each execution get unique hashes and thus track the process
independently even if the same options.args were provided (such as when
running the target but on many different projects)

A bug was introduced with that change, where the hashedKey was created as a
new array on each invocation and the WeakMap would never match. That was fixed
shortly after by using JSON.stringify()

However, even after that fix the hashes were still not quite unique. The
hashedKeys stored in the hashedMap were, but the hashed which was used to
key the processMap would still be the same. In effect this would mean the goal
of being able to call @nrwl/js:node was still not working.

A use case where I am invoking executors like this:
https://github.com/brandingbrand/flagship/blob/main/libs/orchestrate-nx/src/executors/run-all/impl.ts

It is the same idea as the @angular/devkit:all-of
https://github.com/angular/angular-cli/blob/main/packages/angular_devkit/architect/builders/all-of.ts

Current Behavior

When running more than one @nrwl/js:node the process ids will colide causing
the executor to end some processes early and to duplicate others depending on
the order which the @nrwl/js:node was called.

Expected Behavior

Each call of @nrwl/js:node should be unique and track it's processes
completely independently.

Related Issue(s)

#13813
#13908

@vercel
Copy link

vercel bot commented Jan 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
nx-dev ⬜️ Ignored (Inspect) Jan 9, 2023 at 4:18PM (UTC)

@wSedlacek
Copy link
Contributor Author

I don't think the error is related to this change.

FAIL e2e-nx-misc e2e/nx-misc/src/watch.test.ts (138.212 s)
  ● Nx Commands › should watch selected projects only

    expect(received).toEqual(expected) // deep equality

    - Expected  - 4
    + Received  + 1

    - Array [
    -   "proj14124378",
    -   "proj32212199",
    - ]
    + Array []

      86 |     createFile(`newfile2.txt`, 'content');
      87 |
    > 88 |     expect(await getOutput()).toEqual([proj1, proj3]);
         |                               ^
      89 |   });
      90 |
      91 |   it('should watch projects including their dependencies', async () => {

      at src/watch.test.ts:88:31
      at fulfilled (../../node_modules/tslib/tslib.js:115:62)

Test Suites: 1 failed, 3 passed, 4 total
Tests:       1 failed, 38 passed, 39 total
Snapshots:   0 total
Time:        656.259 s
Ran all test suites.

@AgentEnder AgentEnder force-pushed the hotfix/unique-node-exec-keys branch from 8bf973d to 090d10b Compare January 9, 2023 16:18
@AgentEnder
Copy link
Member

@wSedlacek I rebased and kicked off CI again. We will see if it passes this time.

@AgentEnder AgentEnder merged commit 8700c86 into nrwl:master Jan 9, 2023
@wSedlacek wSedlacek deleted the hotfix/unique-node-exec-keys branch January 9, 2023 18:39
@wSedlacek
Copy link
Contributor Author

Thank you for getting this change in! It means a lot to me!

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants