Fix require symlinks for yarn workspaces #15
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15 +/- ##
=========================================
+ Coverage 89.46% 89.5% +0.03%
=========================================
Files 10 10
Lines 940 943 +3
=========================================
+ Hits 841 844 +3
Misses 99 99
Continue to review full report at Codecov.
|
|
@guybedford What am I missing here? |
| const real = fs.realpathSync(resolved); | ||
| if (resolved !== real) { | ||
| // this is likely a symlink | ||
| job.emitFile(resolved, 'symlink', parent); |
There was a problem hiding this comment.
If you emit the path including the symlink, you probably don't need to also emit the real path (because the real path is not required).
There was a problem hiding this comment.
I don't understand.
Both the symlink and the real file need to be emitted because the symlink is just a pointer to the real file. Both are necessary.
There was a problem hiding this comment.
For example, you could have this path (not real) /symlink-folder/file and the real path is /folder/file.
If you add /symlink-folder/file to the file list, you're not actually listing anything real (not even a file that contains a pointer).
That means that:
- the consumer of the list (eg.
@now/node) would need to follow symlinks when copying files - the consumer of the list doesn't need to copy the real file anymore
- you avoid having to detect or copy symlinks. You create real files instead of "virtual" ones when you copy.
There was a problem hiding this comment.
I think I see what you mean. My code is immediately emitting the symlink even though there are some cases when the real file will not be emitted (starts with 'node:`, etc).
@guybedford Do you have a solution for this one?
There was a problem hiding this comment.
It will always be a symlink on this code path because of the early return for the node: URL format.
Note that a resolve-dependency is called as part of the emitDependency routine before it itself calls emitFile. Any file emitted by resolve-dependency means that file was necessary for the resolution process, not that that was the resolved file. The emitFile capability was added to resolve-dependency specifically just to emit package.json files (and so this way we don't emit all package.json files, only those that are actually needed by resolution). Emitting package.json + symlinks exactly fits the model.
There was a problem hiding this comment.
I'm talking about the case where the symlink is directed at a folder and not a file (which is what yarn workspaces is doing). In that case, with the code above, you emit both a "virtual" file and a real file. Is that the expected behaviour? Could you add a test for that?
There was a problem hiding this comment.
The Node.js resolver as written here will only ever resolve a file, not a folder. The statting specifically checks for file existence.
There was a problem hiding this comment.
I confirmed this fixes yarn workspaces with an integration test here: https://github.com/zeit/now-builders/pull/759
There was a problem hiding this comment.
Sorry, I explained it wrong. I'm talking about the case where the file itself is not the symlink, but the symlink is located higher in the folder structure. In this case, if we emit the "virtual" file path, we don't need to emit the real file path.
It works but I think we emit some files twice when only one is necessary 🤔
There was a problem hiding this comment.
Right, that makes complete sense, but this project is just a "tracer" of the file system. It is up to now node to correctly emit the traced filesystem. And yes, it doesn't due to a lack of symlink support both for files and deeper folders as you describe.
| const real = fs.realpathSync(resolved); | ||
| if (resolved !== real) { | ||
| // this is likely a symlink | ||
| job.emitFile(resolved, 'symlink', parent); |
There was a problem hiding this comment.
It will always be a symlink on this code path because of the early return for the node: URL format.
Note that a resolve-dependency is called as part of the emitDependency routine before it itself calls emitFile. Any file emitted by resolve-dependency means that file was necessary for the resolution process, not that that was the resolved file. The emitFile capability was added to resolve-dependency specifically just to emit package.json files (and so this way we don't emit all package.json files, only those that are actually needed by resolution). Emitting package.json + symlinks exactly fits the model.
styfle
changed the title
Fixes #14