Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

feature: add support for additional cache keys option #304

Closed
wants to merge 1 commit into from

Conversation

Rohit-L
Copy link

@Rohit-L Rohit-L commented May 28, 2018

PR for issue #303

This adds an option for user to pass additional keys to the default cache key generator for more customization, allowing the user to have more control, if needed, over what hits/misses in the cache.

@jsf-clabot
Copy link

jsf-clabot commented May 28, 2018

CLA assistant check
All committers have signed the CLA.

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.

Two small fix 👍 Very good works! Thanks!

README.md Outdated
new UglifyJsPlugin({
cache: true,
additionalCacheKeys: {
env: 'development',
Copy link
Member

Choose a reason for hiding this comment

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

Bad example, many people can use this from README, just use myCustomVariable name or something like

const plugin = new UglifyJsPlugin({
cache: true,
additionalCacheKeys: {
env: 'development',
Copy link
Member

Choose a reason for hiding this comment

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

Please also use other variable

@Rohit-L
Copy link
Author

Rohit-L commented May 28, 2018

👍 Updated with suggestions from @evilebottnawi

Copy link
Contributor

@ooflorent ooflorent left a comment

Choose a reason for hiding this comment

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

LGTM. Not a fan of the complex default keys documentation. I think the README does not require so many details about the plugin internals.

@alexander-akait
Copy link
Member

@ooflorent i am not fan also, but it should be documented because in some case it is only one variant to invalidate cache

@shellscape
Copy link

In the past I've taken large, complex portions of a README and moved them out into separate .md files in a ./docs directory. That might be considered to keep the README a little smaller. I would like to review this PR but I know practically nothing about how the plugin works, so I'm afraid that my review wouldn't be very useful.

@alexander-akait
Copy link
Member

@shellscape i think we can do this in other PR, not here

@Rohit-L
Copy link
Author

Rohit-L commented May 29, 2018

@evilebottnawi @ooflorent what do you think about making additionalCacheKeys a function that returns the object of additional keys instead?

I just realized, for my use-case, I need access to file when defining my additional cache keys and this implementation wouldn't allow that.

I'm proposing something similar to how cache-loader defines their cacheKey option.

@alexander-akait
Copy link
Member

alexander-akait commented Jun 8, 2018

@Rohit-L can you provide example? Go ahead 👍 We need this option to implement other feature - custom minify function for support uglify-js and terser

@alexander-akait
Copy link
Member

/cc @Rohit-L friendly ping

@alexander-akait
Copy link
Member

/cc @Rohit-L universal solution #320

@alexander-akait
Copy link
Member

Close in favor #320

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.

None yet

5 participants