--link-duplicates flag. Creates hardlinks to duplicate files in node_modules#2620
Conversation
|
Works on Mac and Windows 10 NTFS. |
|
How does this relate to #2306? Is the hoisting algorithm changed, as well? |
|
It does not implement #2306, we'll need to improve hoisting algorithm anyway. |
|
WOW. |
|
If we don't find any bad side effects I think we could enable it by default everywhere. |
Daniel15
left a comment
Daniel15
left a comment
There was a problem hiding this comment.
This looks pretty good to me :)
| // custom concurrency logic as we're always executing stacks of 4 queue items | ||
| // at a time due to the requirement to push items onto the queue | ||
| while (queue.length) { | ||
| const items = queue.splice(0, 4); |
There was a problem hiding this comment.
Maybe pull out the 4 as a constant (const CONCURRENT_QUEUE_ITEMS = 4 or something like that, that explains what it means)
| } | ||
|
|
||
| // correct hardlink | ||
| if (bothFiles && (srcStat.ino === destStat.ino)) { |
There was a problem hiding this comment.
Does this work on Windows given Windows doesn't use inodes? Maybe this should check that srcStat.ino is not null, just in case?
There was a problem hiding this comment.
Indeed there were some issues with ino on Windows back in Node 0.10 and 0.12 times nodejs/node-v0.x-archive#2670.
The tests on Appveyor pass though.
I'll google a bit more to double check if we can rely on ino not being null
| await promisify(fs.writeFile)(path, data); | ||
| } | ||
|
|
||
| export async function hardlinksWork(cwd: string): Promise<boolean> { |
There was a problem hiding this comment.
Maybe just call the param dir, as it could be called with any directory - It doesn't have to be the current directory.
| /* @flow */ | ||
|
|
||
| import {getPackageVersion, runInstall} from '../_helpers.js'; | ||
| import * as fs from '../../../src/util/fs.js'; |
There was a problem hiding this comment.
why do you need to do * as here?
There was a problem hiding this comment.
You mean I could only import stat instead of * as fs?
Not sure if that would be better when the number of imports from fs grow.
Everything grouped into one import makes sense rather than a lot functions that need sorting and formatting.
What should be the guideline here?
|
Updated with fixes |
|
I'm going to work on a prepass to optimise module hoisting today, this will go nicely in conjunction with it. Thank you so much @bestander! |
…modules (yarnpkg#2620) * Feature: duplicated dependencies are hardlinked from the first copy * cleanup and tests * made linking non default * added fallback if links are not supported * lint fix * nits * nits
ghost
mentioned this pull request
|
As this tweet indicates, this may not make it to v21 final - anecdotally, this flag got our React app's install size down by 20%, whereas #2676 only reduced it by about 2%. Unfortunately,
Please indicate if you'd like me to move this to a separate issue. |
|
@STRML, if there this feature gives benefits to the community then we will keep it, thanks for speaking up. |
|
Sure, I'll try to get a minimal repo going then open the issue. |
7 participants
Summary
An attempt to mitigate inefficient hoisting for create-react-app and #2306.
Inspired by https://github.com/jeffbski/pkglink, instead of running copy for all repeated modules in node_modules this algorithm does only one copy and then hardlinks every file in the duplicate modules.
The win is in speed and size.
Test plan