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

feat(repo): upgrade to latest version of Nx #22574

Closed
wants to merge 7 commits into from
Closed

Conversation

mandarini
Copy link
Contributor

@mandarini mandarini commented May 16, 2023

NOTE: Maybe use this updated PR instead: #22694

What I did

Updated repo to latest Nx.

How to test

I upgraded and then ran yarn task and compiled all projects, all projects compiled successfully.

Note:

We can remove migrations.json, I left it there for now, for you to see what scripts it ran! :)

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@mandarini mandarini self-assigned this May 16, 2023
@mandarini mandarini marked this pull request as ready for review May 16, 2023 12:55
@socket-security
Copy link

socket-security bot commented May 16, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎


🚨 Potential security issues found in this pull request. To accept the risk, merge this PR and you will not be notified again.

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore @nx/devkit@16.1.4
  • @SocketSecurity ignore @nx/devkit@16.2.1
  • @SocketSecurity ignore @nx/workspace@16.1.4
  • @SocketSecurity ignore @nx/workspace@16.2.1
  • @SocketSecurity ignore nx-cloud@16.0.5
⚠️ Shell access

This module accesses the system shell. Accessing the system shell increases the risk of executing arbitrary code.

Packages should avoid accessing the shell which can reduce portability, and make it easier for malicious shell access to be introduced.

Package Module Location Source
@nx/devkit@16.1.4 (added) child_process src/tasks/install-packages-task.js code/package.json via @nx/workspace@16.1.4
@nx/devkit@16.1.4 (added) child_process src/utils/package-json.js code/package.json via @nx/workspace@16.1.4
@nx/devkit@16.2.1 (added) child_process src/tasks/install-packages-task.js scripts/package.json via @nx/workspace@16.2.1
@nx/devkit@16.2.1 (added) child_process src/utils/package-json.js scripts/package.json via @nx/workspace@16.2.1
@nx/workspace@16.1.4 (added) child_process src/generators/new/generate-preset.js code/package.json
@nx/workspace@16.1.4 (added) child_process src/generators/utils/get-npm-package-version.js code/package.json
@nx/workspace@16.1.4 (added) child_process src/utilities/default-base.js code/package.json
@nx/workspace@16.2.1 (added) child_process src/generators/new/generate-preset.js scripts/package.json
@nx/workspace@16.2.1 (added) child_process src/generators/utils/get-npm-package-version.js scripts/package.json
@nx/workspace@16.2.1 (added) child_process src/utilities/default-base.js scripts/package.json
nx-cloud@16.0.5 (added) child_process lib/core/commands/record-output.js code/package.json, scripts/package.json
nx-cloud@16.0.5 (added) child_process lib/utilities/environment.js code/package.json, scripts/package.json
Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ⚠️ 12 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
nx-cloud@16.0.5 filesystem, shell, environment +4 altan-nrwl
@nx/workspace@16.1.4 filesystem, shell, environment +14 nrwl-jason
@nx/workspace@16.2.1 filesystem, shell, environment +14 nrwl-jason
nx@16.1.4 environment +10 nrwl-jason
nx@16.2.1 environment +10 nrwl-jason

🚮 Removed packages: @nrwl/nx-cloud@15.3.5, @nrwl/workspace@15.9.4

@chakAs3 chakAs3 self-assigned this May 16, 2023
@chakAs3 chakAs3 requested review from chakAs3 and IanVS May 16, 2023 17:30
@chakAs3
Copy link
Contributor

chakAs3 commented May 16, 2023

hi @mandarini thanks for your work.
I'm testing it now, indeed the build passes, However, i'm trying to run some scripts, to create and run the sandboxes.


⚙️ Initializing Storybook
> node /Users/chakir/devs/storybook/code/lib/cli/bin/index.js init --yes

for react yarn start it is stuck here never finished.
for vue3-vite it works very fast

@mandarini
Copy link
Contributor Author

@chakAs3 can you tell me what command you are running exactly, and in which directory?

@chakAs3
Copy link
Contributor

chakAs3 commented May 17, 2023

@chakAs3 can you tell me what command you are running exactly, and in which directory?
root directory

yarn task --task dev --template react-vite/default-ts --start-from=install

it is stuck in prompts asking some plugin install, the promise is not returned

if i bypass the prompts it passes .

@mandarini
Copy link
Contributor Author

mandarini commented May 18, 2023

@chakAs3 yes ok I just verified. Weird, I wonder what it can be. I'm looking into it. In any case, I updated the scripts directory too

@mandarini
Copy link
Contributor Author

mandarini commented May 18, 2023

@chakAs3 same thing happens to me in latest next, in the existing version of Nx, before migrating. So I guess that's not an issue with the migration... And it does not look like an issue with Nx either, I believe. :( It just hangs there forever.

Screenshot 2023-05-18 at 11 58 49 AM

@chakAs3
Copy link
Contributor

chakAs3 commented May 18, 2023

@chakAs3 same thing happens to me in latest next, in the existing version of Nx, before migrating. So I guess that's not an issue with the migration... And it does not look like an issue with Nx either, I believe. :( It just hangs there forever.

Screenshot 2023-05-18 at 11 58 49 AM

You are right it looked not related to NX anyway, i have even upgraded the'prompts' package, even replace with @clack/prompts but it is same just stuck there

@mandarini
Copy link
Contributor Author

I see! Let me know what you think about updating Nx, in any case! :)

@shilman
Copy link
Member

shilman commented May 19, 2023

@chakAs3 This was a CLI change that was introduced last week. I believe @yannbf and @ndelangen will have a fix today

@chakAs3
Copy link
Contributor

chakAs3 commented May 20, 2023

@chakAs3 This was a CLI change that was introduced last week. I believe @yannbf and @ndelangen will have a fix today

yes it was the prompts, it was fixed with a workaround to unblock the contributors.

Copy link
Contributor

@chakAs3 chakAs3 left a comment

Choose a reason for hiding this comment

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

I revert back to nx@16.2.1 as the beta creating some issue for with type checking

@chakAs3 chakAs3 self-requested a review May 22, 2023 13:41
@chakAs3
Copy link
Contributor

chakAs3 commented May 22, 2023

Hi @mandarini i hope you are having a good week start.

Unfortunately i could not merge your PR, I'm getting plenty of type checking errors, so CI tests won't pass
to test run yarn ci-tests or yarn task --task check --no-link

@mandarini
Copy link
Contributor Author

Hi @chakAs3 ! I will rebase my PR, squash the commits and push again if that's ok with you. I re-applied the latest upgrade, just to see the effects after rebasing with latest next. You can ping me on slack/discord if you want to briefly pair on this today. The tests are passing on my end, but I understand that "works on my machine" does not say much, hehe! :)

@mandarini
Copy link
Contributor Author

@chakAs3 @shilman I created a new PR from scratch, and it seems to work for me, but let's test again! #22694

@ndelangen
Copy link
Member

I suppose the new PR replaces this one?

@ndelangen ndelangen closed this May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants