Skip to content

Conversation

@agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Aug 25, 2022

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
- 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 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

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

Development

Successfully merging this pull request may close these issues.

2 participants