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

feature: use disk file cache #3275

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yejinjian
Copy link

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

can use disk file cache when rollup watch

@codecov
Copy link

codecov bot commented Dec 8, 2019

Codecov Report

Merging #3275 into master will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3275      +/-   ##
==========================================
- Coverage   93.12%   93.09%   -0.04%     
==========================================
  Files         170      170              
  Lines        5967     5969       +2     
  Branches     1780     1781       +1     
==========================================
  Hits         5557     5557              
- Misses        219      220       +1     
- Partials      191      192       +1     
Impacted Files Coverage Δ
src/watch/index.ts 83.78% <0.00%> (-1.54%) ⬇️

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 b5a3f16...2450575. Read the comment docs.

@lukastaegert
Copy link
Member

Hi, thanks for the PR. Could you outline how this enables users to use a disk cache, or more general, how this feature should be used? If we want this feature, then it would need to be documented, possibly with an example as I believe it will be non-trivial. Also, even though you checked the box that there are tests you did not add any, which means the coverage check fails. I may be able to help you with the latter, but first it would be important to see an example usage.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Also see the longer comment I left in the discussion. Mostly, we need tests and documentation.

@@ -139,6 +139,10 @@ export class Task {
});
this.inputOptions = inputOptions;

if (config && config.cache) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible the config is not an object? Otherwise the check can be shortened.

Copy link
Author

Choose a reason for hiding this comment

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

you are right, it my habitual behavior.

@shrinktofit
Copy link

shrinktofit commented Dec 25, 2019

I would have a negative attitude to "use disk cache" if .cache generated by rollup() is not going to be optimized...

For project like Cocos Creator 3D, the .cache is too large(341,022,545 chars after JSON.stringify()) to store and load.

@shellscape
Copy link
Contributor

@shrinktofit I wouldn't think in today's day and age that a 300mb cache would be too large.

@lukastaegert is there a hook that could be used to compress/decompress this cache file? I'm not sure I think the added dependency on something that could gzip, etc would be worth the increase in size to the rollup core bundle. But for those who have massive cache files, it could be useful to provide a path to reducing the file size.

@lukastaegert
Copy link
Member

No, and contrary to the title, this PR DOES NOT add a disk cache to Rollup, which is why any discussion around this is not really relevant to this PR. As I understand it it enables users of the JS API to hook in their own (disk or otherwise) cache by properly restoring from a given cache object. Which is why I was asking for an example code how this could be used to actually have a disk cache and documentation in general.

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

4 participants