Skip to content
This repository was archived by the owner on Oct 27, 2020. It is now read-only.

Relative paths #37

Closed
wants to merge 2 commits into from
Closed

Conversation

gdborton
Copy link

@gdborton gdborton commented Jul 9, 2018

Commits:
Adds a build:watch npm script.

I was working on cache-loader as an npm linked dependency, I generally use this
script so that I don't have to think about rebuilding between changes.

Adds a projectRoot option to enable relative cache paths.

Previous to this change cache keys (and the values stored in the cache) were
using absolute paths. This meant that moving a buildable repository to a
different directory would entirely invalidate the cache, and that the cache
wouldn't be shareable between builds in CI when building on different machines.

This change adds a new option projectRoot that helps to make the paths
relative, and therefore reusable when the project changes directories.

This is my attempt at addressing: #36

I was working on cache-loader as an npm linked dependency, I generally use this
script so that I don't have to think about rebuilding between changes.
@jsf-clabot
Copy link

jsf-clabot commented Jul 9, 2018

CLA assistant check
All committers have signed the CLA.

Previous to this change cache keys (and the values stored in the cache) were
using absolute paths. This meant that moving a buildable repository to a
different directory would entirely invalidate the cache, and that the cache
wouldn't be shareable between builds in CI when building on different machines.

This change adds a new option `projectRoot` that helps to make the paths
relative, and therefore reusable when the project changes directories.
@codecov
Copy link

codecov bot commented Jul 9, 2018

Codecov Report

Merging #37 into master will decrease coverage by 0.51%.
The diff coverage is 6.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
- Coverage   17.47%   16.96%   -0.52%     
==========================================
  Files           2        2              
  Lines         103      112       +9     
  Branches       13       13              
==========================================
+ Hits           18       19       +1     
- Misses         73       81       +8     
  Partials       12       12
Impacted Files Coverage Δ
src/index.js 17.11% <6.66%> (-0.53%) ⬇️

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 ebd53aa...9aee709. Read the comment docs.

@gdborton
Copy link
Author

gdborton commented Jul 9, 2018

I'm not sure if I should be correcting the coverage... seems like coverage is super low already and I'd have to write the tests for basically all of the repo. I guess I could move the few new functions to their own file and unit test that way.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Why don't use context option? We already known project dir. Also this implementation decrease perf.

@gdborton
Copy link
Author

You're talking about webpack's context config right? This is usually a path that's nested in the repo and not at the root.

On a local test repo for example, this.context could be:
/Users/gary_borton/airlab/repos/test-cache-loader-1/src

which wouldn't be contained in:
/Users/gary_borton/airlab/repos/test-cache-loader-1/node_modules/lib/index.js

For the performance, do you have any recommendations? I think the runtime performance hit is worth the cache being reusable across build machines as without it the cache is effectively worthless.

After digging into this a bit, I think that we can fix it on our end via config, but it'd be nice if cross machine/directory caching just worked for users.

@gdborton
Copy link
Author

@evilebottnawi any thoughts on the above?

@gdborton
Copy link
Author

@evilebottnawi ping?

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #37 into master will decrease coverage by 0.51%.
The diff coverage is 6.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
- Coverage   17.47%   16.96%   -0.52%     
==========================================
  Files           2        2              
  Lines         103      112       +9     
  Branches       13       13              
==========================================
+ Hits           18       19       +1     
- Misses         73       81       +8     
  Partials       12       12
Impacted Files Coverage Δ
src/index.js 17.11% <6.66%> (-0.53%) ⬇️

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 ebd53aa...9aee709. Read the comment docs.

@mistic
Copy link
Collaborator

mistic commented Dec 18, 2018

@evilebottnawi this can be closed in favor of #49

@alexander-akait
Copy link
Member

Close in favor #49

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

Successfully merging this pull request may close these issues.

4 participants