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

Tool 1100 yarn workspace support #33

Merged
merged 11 commits into from Oct 10, 2019
Merged

Conversation

lpusok
Copy link
Contributor

@lpusok lpusok commented Oct 4, 2019

If finding a symlink, we just ignore it as far as cache invalidation goes.
Yarn workspaces can create symlinks to directories outside of the node_modules folder, and do not want to follow these.
If the symlink (file or directory) points inside the cached folder, then I also do not want to follow those, as the files will be part of the cache anyway.
Yarn cache creates symlinks to executables (this seems to work currently)

Yarn Workspaces, this does not work:
https://yarnpkg.com/lang/en/docs/workspaces/
https://yarnpkg.com/blog/2018/02/15/nohoist/

❯❯❯ ls -al node_modules
total 8
drwxr-xr-x   5 lpusok  staff  160 Oct  1 18:44 .
drwxr-xr-x  10 lpusok  staff  320 Oct  1 18:44 ..
-rw-r--r--   1 lpusok  staff  274 Oct  1 18:44 .yarn-integrity
lrwxr-xr-x   1 lpusok  staff   13 Oct  1 18:44 a -> ../packages/a
lrwxr-xr-x   1 lpusok  staff   13 Oct  1 18:44 b -> ../packages/b

@codecov-io
Copy link

codecov-io commented Oct 4, 2019

Codecov Report

Merging #33 into master will decrease coverage by 5.27%.
The diff coverage is 79.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   48.32%   43.05%   -5.28%     
==========================================
  Files           6        6              
  Lines         478      432      -46     
==========================================
- Hits          231      186      -45     
  Misses        216      216              
+ Partials       31       30       -1
Impacted Files Coverage Δ
cache_descriptor.go 79.71% <100%> (ø) ⬆️
cache_path.go 81.65% <78.26%> (-4.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 998a01f...c79a973. Read the comment docs.

Copy link
Contributor

@godrei godrei left a comment

Choose a reason for hiding this comment

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

Pls check my notes

cache_path.go Outdated Show resolved Hide resolved
cache_path.go Outdated
// If case it a link to a directory outside of the cached paths (e.g. yarn workspaces),
// will not add the linked directory to the cache, and will not invalidate cache if it changes.
// If it links to a directory included in the cache already, then also ignoring it.
// The directory contents will be added to the cache as regular files, so not need to check them twice.
Copy link
Contributor

Choose a reason for hiding this comment

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

so not need to check -> no need to check

cache_path.go Outdated
return err
}

// Ignoring symlink target changes for cache invalidation.
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this comments could be added to the file level comment block, as it is not strictly related to this code block only, but a more general functional description of this file.

cache_path.go Outdated Show resolved Hide resolved
cache_path.go Outdated
}

// Ignoring symlink target changes for cache invalidation.
// If case it a link to a directory outside of the cached paths (e.g. yarn workspaces),
Copy link
Contributor

Choose a reason for hiding this comment

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

In case it is a link ...

cache_path.go Outdated Show resolved Hide resolved
@lpusok lpusok requested a review from godrei October 9, 2019 12:57
@godrei godrei merged commit b966f9d into master Oct 10, 2019
@godrei godrei deleted the TOOL-1100-yarn-workspace-support branch October 10, 2019 13:07
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.

None yet

3 participants