Update node modules installation check to npm exclusively #14
Conversation
|
@franciszekjob is attempting to deploy a commit to the software-mansion Team on Vercel. To accomplish this, @franciszekjob needs to request access to the Team. Afterwards, an owner of the Team is required to accept their membership request. If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account. |
| const isTreeVerified = await checkIfCLIInstalled( | ||
| `yarn check --verify-tree --cwd ${getAppRootFolder()}` | ||
| ); | ||
| const isIntegrityChecked = await checkIfCLIInstalled( | ||
| `yarn check --integrity --cwd ${getAppRootFolder()}` | ||
| ); |
There was a problem hiding this comment.
The yarn check command is deprecated and highly discouraged by the official yarn documentation
https://classic.yarnpkg.com/lang/en/docs/cli/check/
It also says that
The switches
--integrityand--verify-treeare mutually exclusive.
so i'm not sure whether we should use both of these at the same time
I'm starting to lean on just using npm list for every package manager check as it's unlikely a user wouldn't have npm installed
There was a problem hiding this comment.
Ok, should I implement it the way it relies only on npm list then?
There was a problem hiding this comment.
The docs suggest using https://classic.yarnpkg.com/lang/en/docs/cli/install/#toc-yarn-install-check-files instead. Maybe try yarn install --check-files first if that isn't good enough fallback to npm list.
There was a problem hiding this comment.
yarn install --check-files installs node modules as well, which is an unwanted behavior in this case. Maybe for now we can exclusively use npm for all cases? npm would then become a requirement for RN IDE
There was a problem hiding this comment.
i think npm is always installed with node either way so its okay to assume everybody has npm installed
franciszekjob
changed the title
franciszekjob
changed the title
franciszekjob
changed the title
This PR removes code responsible for node_modules checks. These checks have been buggy and caused many projects to report issues while not giving a lot of value. Specifically, yarn-base projects would occasionally fail the check (maybe due to packages that were incompatible with node). On the other hand, we couldn't use project-specific package manager to perform this check because for example yarn wouldn't report a problem even when node_modules folder didn't exist (see #14) We may want to revisit this in the future but it'd need to provide some additional values such as: 1) verifying if node_modules are in sync with current package.json and lock files – the old implementation only checked the existence of node_modules 2) the command should be fast, such that we can perform this verification at different moments, for example when we suspect someone should re-run install command (i.e. they checkout a different branch with changes to lockfiles)
This PR restricts listing installed node modules to
npmonly. At the same time it fixes the issue which marke node modules as installed even if they were not present.Before:
The check to verify packages was performed using different package managers.
After:
Now we use only
npmto determine the presence of node modules.npmis included withnodeso there's no risk that user won't have it.