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.

Sign up for GitHub

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

Jump to bottom

refactor(cache): simplify creating / using the cache var #415

Merged
merged 1 commit into from Aug 30, 2022
Merged

refactor(cache): simplify creating / using the cache var #415

merged 1 commit into from Aug 30, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Aug 25, 2022 •
edited

NOTE: this is built on top of #414 as it uses the buildStart changes made there. As such, I've marked this PR as "Draft" until #414 is merged.
Rebased on top and marked as ready for review.

Summary

Use the buildStart hook added in #414 to simplify cache() to cache

Details

  • as everything is created in the buildStart hook now (which has RollupContext), we can create the cache there too

    • no need for slightly hacky, lazy creation during transform anymore
    • simplifies it and also standardizes it so it's created the same way as all the other instance vars

  • fix: reset cache after each watch cycle

    • previously the cache was never reset, meaning that if anything became dirty in a watch cycle, it would never get reset back
      • I noticed this in the processing of writing watch mode tests in test: add initial watch mode test suite #386
      • in some cases, this would mean that the cache was effectively always dirty during an entire watch mode run, and therefore never used
        • this would be quite inefficient as the FS usage for the cache would just go to waste
    • see that test coverage has now increased as a result!

@agilgur5 agilgur5 added kind: bug Something isn't working properly kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: cache Related to the cache scope: watch mode Related to Rollup's watch mode labels Aug 25, 2022
@agilgur5
refactor(cache): simplify creating / using the cache var
0dabda3 
- as everything is created in the `buildStart` hook now (which has `RollupContext`), we can create the `cache` there too
  - no need for slightly hacky, lazy creation during `transform` anymore
  - simplifies it and also standardizes it so it's created the same way as all the other instance vars

- fix: reset `cache` after each watch cycle
  - previously the cache was never reset, meaning that if anything became dirty in a watch cycle, it would never get reset back
    - in some cases, this would mean that the cache was effectively always dirty during an entire watch mode run, and therefore never used
      - this would be quite inefficient as the FS usage for the cache would just go to waste
  - see that test coverage has now increased as a result!
@agilgur5 agilgur5 force-pushed the refactor-cache-simplify-create branch from 2fcae7c to 0dabda3 Compare August 29, 2022 22:07
@agilgur5 agilgur5 marked this pull request as ready for review August 29, 2022 22:08
@agilgur5 agilgur5 requested a review from ezolenko August 30, 2022 01:57
@ezolenko ezolenko merged commit c6be0eb into ezolenko:master Aug 30, 2022
@agilgur5 agilgur5 mentioned this pull request Sep 17, 2022
Merged
@agilgur5 agilgur5 deleted the refactor-cache-simplify-create branch July 2, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezolenko ezolenko Awaiting requested review from ezolenko

Assignees
No one assigned
Labels
kind: bug Something isn't working properly kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: cache Related to the cache scope: watch mode Related to Rollup's watch mode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@agilgur5 @ezolenko