Skip to content
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.

Sign up for GitHub

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 require symlinks for yarn workspaces #15

Merged
merged 11 commits into from
Jul 13, 2019
Merged

Fix require symlinks for yarn workspaces #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
617d697
Fix symlink emission for yarn workspaces
styfle Jul 12, 2019
6165cb4
Simplify logic
styfle Jul 12, 2019
f540887
Add reason to emit file
styfle Jul 12, 2019
d4b8594
Add unit test fs-emission-symlink
styfle Jul 12, 2019
8db682d
Add missing test result
styfle Jul 12, 2019
0775a83
Fix test output
styfle Jul 12, 2019
ee34f86
Add path.join()
styfle Jul 12, 2019
15871ec
Fix test to use require
styfle Jul 12, 2019
d2b2fd4
Fix output
styfle Jul 13, 2019
f8d3070
Rename test to require-symlink
styfle Jul 13, 2019
ba73e33
Remove comment since this is always a symlink
styfle Jul 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/resolve-dependency.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ module.exports = function resolveDependency (specifier, parent, job) {
else
resolved = resolvePackage(specifier, parent, job);
if (resolved.startsWith('node:')) return resolved;
return fs.realpathSync(resolved);
const real = fs.realpathSync(resolved);
if (resolved !== real) {
job.emitFile(resolved, 'symlink', parent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lucleray lucleray Jul 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. the consumer of the list (eg. @now/node) would need to follow symlinks when copying files
  2. the consumer of the list doesn't need to copy the real file anymore
  3. you avoid having to detect or copy symlinks. You create real files instead of "virtual" ones when you copy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Node.js resolver as written here will only ever resolve a file, not a folder. The statting specifically checks for file existence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed this fixes yarn workspaces with an integration test here: https://github.com/zeit/now-builders/pull/759

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return real;
};

function resolvePath (path, parent, job) {
Expand Down
1 change: 1 addition & 0 deletions test/unit/require-symlink/another.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = () => console.log('hello');
1 change: 1 addition & 0 deletions test/unit/require-symlink/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('./symlink')
5 changes: 5 additions & 0 deletions test/unit/require-symlink/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[
"require-symlink/another.js",
"require-symlink/input.js",
"require-symlink/symlink"
]
1 change: 1 addition & 0 deletions test/unit/require-symlink/symlink
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
another.js