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

Added option for custom cache hash behavior #655

Closed

Conversation

saionaro
Copy link

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

No option for setting custom cache hash behavior

What is the new behavior?

Added option for setting custom cache hash behavior.

const fields = {
  filename: 1,
  sourceRoot: 1,
  dirname: 1,
  resolved: 1,
  cwd: 1
};
...
modifyCacheHash: (source, options, cacheIdentifier) => {
  const convertedOptions = JSON.stringify(options, (key, value) => {
    if (fields[key]) {
      return undefined;
    }
    return value;
  });
  return source + convertedOptions + cacheIdentifier;
}
...

Does this PR introduce a breaking change?

  • Yes
  • No

Other information:

It can be useful if your builds are deploying on Heroku and a build directory is dynamic (for example /tmp/build<random-hash>/). In this case, we should exclude the absolute build path from the hash string to hit cache stored by Heroku.

@saionaro
Copy link
Author

Hello, @hzoo ! Please, take a look - WDYT?

@suchipi
Copy link
Member

suchipi commented Aug 23, 2018

This is only needed in a very specific use-case, and I'm not sure it makes sense to merge in...

@saionaro
Copy link
Author

Builds in a temporary directory is not a very specific use case I think.
Same Heroku popular for project hosting and it has this build way by default.

@edmorley
Copy link

edmorley commented Aug 23, 2018

Indeed, with Heroku (or any other deployment or CI service that both varies the build directory per job), without this feature the cache is useless.

@lencioni opened #637 to handle this in a different way, so it seems like there is demand. (And I suspect that many people don't realise their caches are being busted, and just think the cache feature isn't that effective.)

@saionaro
Copy link
Author

@loganfsmyth can you take a look at it?

@loganfsmyth
Copy link
Member

loganfsmyth commented Sep 1, 2018

I think my feeling here is the same as #637 (comment) We could add a cacheRoot or something if you want, and automatically strip that out. There's still always the risk that it'll go wrong though. It really seems like something Heroku should be resolving using chroot or something so the builds run in a consistent directory location.

@loganfsmyth
Copy link
Member

Got some useful feedback from someone at Heroku in heroku/heroku-buildpack-nodejs#439 (comment) FYI. If it's not going to happen on their side any time soon, I'm definitely more willing to add a cacheRoot option. Still not a huge fan a generic modifyCacheHash function though.

@saionaro saionaro closed this Dec 18, 2018
@saionaro saionaro deleted the feature/modify-cache-hash-option branch December 18, 2018 17:30
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.

4 participants