-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 npm 7 compatability #28824
Fix npm 7 compatability #28824
Conversation
Thank you for starting this PR. I personally have issues with I really like the new format for the lock file. It puts our local packages on top of the file. I hope it will drastically improve the experience around the deduplication of dependencies. So far, updating packages with npm 6 was a terrible experience in monorepo setup. |
3f05a2a
to
acff485
Compare
Size Change: 0 B Total Size: 1.36 MB ℹ️ View Unchanged
|
I have never encountered this issue 🤔 , maybe it's already been fixed?
Unfortunately, seems like the new lockfile format breaks the CI. Possibly related to npm/cli#2610? Not sure 🤔. Here's the original CI failure message. |
👋 In my case I'm having this issue when running Looks like it's related to Appium dependency but the error is different from the one described 🤔 . I'll try with your fix @kevin940726 just in case it solves it too, thanks! |
const dirList = fs.readdirSync( dir, { withFileTypes: true } ); | ||
const packageDirs = []; | ||
dirList.forEach( ( packageName ) => { | ||
const packageDir = path.join( dir, packageName ); | ||
if ( packageName.startsWith( '@' ) ) { | ||
packageDirs.push( ...fetchRNPackageDirs( packageDir ) ); | ||
} else { | ||
const files = fs.readdirSync( packageDir ); | ||
const podSpecs = files.filter( ( file ) => | ||
file.toLowerCase().endsWith( '.podspec' ) | ||
); | ||
if ( podSpecs.length > 0 ) { | ||
packageDirs.push( { | ||
dir: packageDir, | ||
files: podSpecs, | ||
package: packageName, | ||
} ); | ||
dirList | ||
.filter( ( file ) => file.isDirectory() ) | ||
.map( ( file ) => file.name ) | ||
.forEach( ( packageName ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could omit { withFileTypes: true }
and check for a leading dot .
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's error prone as it would break if npm decides to change that by adding another directory without a leading dot. This patch file is already a hack and we should prevent it from breaking our build again 😅 .
Thanks, this is looking promising, I'll give it a spin! Best viewed with whitespace changes ignored: https://github.com/WordPress/gutenberg/pull/28824/files?diff=split&w=1 |
I found out that the dependencies that have this issue are located in I modify them manually in a commit from the PR I'm trying to fix the Appium dependency and it prevents the issue but runnin I'll investigate further if we can update those forks somehow to prevent this issue because until they fix this issue on NPM we're a bit blocked for applying the fix. |
Great sleuthing, @fluiddot!
Maybe we can try the opposite? I.e. maybe we don't actually need the |
Alternatively, we could try the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working well for me. Thanks @kevin940726!
I tried using |
The final version doesn’t include changes to the lock file. Noting because the title and description of the PR wasn’t updated. |
FWIW, we haven't completely figured out that issue yet, but it's separate from the one with the |
Yeah, but running |
Yep, we shouldn't use NPM 7 yet because it will update the |
What/if any adjustments should I make here? |
I'm not totally sure about which part should be adjusted, probably someone with more context could point it out. I'm wondering if it should be the part that says:
We could change the NPM version conditions to: By the way, there's a related comment to this one. |
Description
Upgrade to supports npm 7. See the changelog here.
This fixes an error when running
npm install
with npm 7:The error is because, in
patches/patch-xcode
, we were assuming every file in thenode_modules
directory is a directory. Which apparently changes now that it also consists a.package-lock.json
file.This PR also upgrades the version of the lock file format to version 2, which should also be backward-compatible to npm 6 users.Follow-up in progress: #28849.How has this been tested?
Types of changes
New feature
Checklist: