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.

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

Do not emit fs.realpath() path of files #20

Closed
wants to merge 4 commits into from
Closed

Conversation

lucleray
Copy link
Member

@lucleray lucleray commented Jul 13, 2019

Following-up #15.

This PRs removes the fs.realpath()'d path of a file in fileList.

Example:

input.js
symlink -> folder
folder/another.js
// folder/another.js
module.exports = () => console.log('hello');
// index.js
require('symlink/another`)

Here I would expect the trace to be input.js and symlink/another.js.
Currently, it is input.js, folder/another.js and symlink/another.js (folder/another.js is not required anywhere!).

Other example:

input.js
symlink -> another.js
another.js
// another.js
module.exports = () => console.log('hello');
// index.js
require('symlink`)

Here I would expect the trace to be input.js and symlink. It's up to the consumer of the trace to chose how it wants to copy the symlink file (either by creating a real file at symlink or by copying the symlink and its target).
Currently it is input.js, symlink and another.js.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

In CommonJS resolution, whenever resolving relative to a path that is symlinked, the resolution must always happen relative to the realpath. To not apply this realpath is to break CJS semantics.

Yes now-node isn't doing the right thing without symlink support, but that means we must fix it there, not break the semantic here.

@@ -1,5 +1,4 @@
[
Copy link
Member

@styfle styfle Jul 13, 2019

Choose a reason for hiding this comment

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

By removing this test, you are effectively breaking yarn workspaces 🤔

I added this test in #15 specifically for yarn workspaces so I can't approve this PR if you are modifying this test.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

This doesn't seem correct because you changed the test I just added in #15

@lucleray
Copy link
Member Author

Got it @guybedford. Then I guess we need to always resolve the real paths, and add the location of the symlink to the file list.

I still think it's wrong to have both the "virtual path" and the real path 🤔 Because the consumer won't now the "virtual path" is from a symlink and will create a real file (lstat on symlink/another.js will say it's a real file!).

@guybedford
Copy link
Contributor

@lucleray my understanding is for example in a git repo if you add a file or folder that is a symlink, that it will embed that symlink into the file model. Or similarly for archiving with tar etc. And yes that involves statting again at the time of constructing the file system model. So we could in theory do that work here, and instead output symlinkList and symlinkDirList as separate lists alongside the main files instead of including them in fileList.

@lucleray
Copy link
Member Author

Got it @guybedford. Thank you for explaining 🙂

@lucleray lucleray closed this Jul 15, 2019
@lucleray lucleray deleted the symlink-folder branch July 15, 2019 20:36
@guybedford
Copy link
Contributor

Implemented #21 as a result of this discussion - thanks @lucleray for working through this one here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants