Fix npm 7 compatability #28824
Fix npm 7 compatability #28824
Conversation
kevin940726
added
[Type] Bug
|
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. |
kevin940726
force-pushed
the
fix/npm-7
branch
from
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.
Alternatively, we could omit { withFileTypes: true } and check for a leading dot . instead.
There was a problem hiding this comment.
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 |
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 installwith npm 7:The error is because, in
patches/patch-xcode, we were assuming every file in thenode_modulesdirectory is a directory. Which apparently changes now that it also consists a.package-lock.jsonfile.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: