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

fix(cache): default clean: true when necessary, add extraCacheKeys option #420

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

fix(cache): default clean: true when necessary, add extraCacheKeys option #420

wants to merge 3 commits into from

Conversation

oscard0m
Copy link

@oscard0m oscard0m commented Sep 10, 2022 •
edited

Summary

Context

Fixes #228

Questions

  • Do you know if this approach is correct? It has been a long time since I do not touch rollup and this plugin, so I want to validate this step before getting deeper.

unless the user provided the extraCacheKeys option, we then leave everything up to the user

I believe the only two are sourceMapCallback and transformers

  • Does this mean I need to do some logic to decide the default value for the clean option depending on these 2 other options?

@oscard0m oscard0m mentioned this pull request Sep 10, 2022
Open
agilgur5
agilgur5 requested changes Sep 10, 2022
View reviewed changes
Copy link
Collaborator

@agilgur5 agilgur5 left a comment •
edited

Choose a reason for hiding this comment

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

Good questions -- you're on the right track with those 🙂

It doesn't exist yet -- this PR would be creating it.

It would be placed in the same index.ts block where you changed clean currently (the Object.assign block sets defaults).

  • Set clean option to true by default

It should not be true by default -- see below

  • Does this mean I need to do some logic to decide the default value for clean option depending on these 2 other options?

Yep. Here's a quick logic flow:

  1. clean is false by default
  2. If sourceMapCallback or transformers are passed in, then it should default to true instead
  3. If extraCacheKeys is passed in, then it should default back to false again

2 should also log out, telling the user to set extraCacheKeys if they want to re-enable the cache.

@agilgur5 agilgur5 changed the title feat(cache): set options.clean as 'true' by default feat(cache): add extraCacheKeys options, default to clean: true when necessary Sep 11, 2022
@agilgur5 agilgur5 changed the title feat(cache): add extraCacheKeys options, default to clean: true when necessary feat(cache): add extraCacheKeys option, default to clean: true when necessary Sep 11, 2022
@agilgur5 agilgur5 added kind: bug Something isn't working properly kind: feature New feature or request scope: cache Related to the cache scope: docs Documentation could be improved. Or changes that only affect docs labels Sep 11, 2022
@agilgur5 agilgur5 changed the title feat(cache): add extraCacheKeys option, default to clean: true when necessary fix(cache): default clean: true when necessary, add extraCacheKeys option Sep 11, 2022
@oscard0m oscard0m force-pushed the set-clean-option-to-true-by-default branch from a196d87 to fa70a2f Compare September 18, 2022 17:07
@oscard0m
Copy link
Author

Applied more changes to the draft.

Now it's not clear to me which part of the code (and how) the extraCacheKeys should be treated.

agilgur5
agilgur5 requested changes Sep 18, 2022
View reviewed changes
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

I'm on my phone rn, so only left a few simpler comments for now.

With regard to where to use extraCacheKeys, it would be in the call to object-hash in tscache, i.e. in this block.

src/ioptions.ts Outdated Show resolved Hide resolved
src/tscache.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
agilgur5
agilgur5 requested changes Sep 30, 2022
View reviewed changes
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

a few other things from my laptop now

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@agilgur5 agilgur5 added the problem: stale Issue has not been responded to in some time label Dec 27, 2022
@oscard0m oscard0m requested a review from agilgur5 January 8, 2023 12:37
@oscard0m oscard0m marked this pull request as ready for review January 8, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agilgur5 agilgur5 Awaiting requested review from agilgur5

Assignees
No one assigned
Labels
kind: bug Something isn't working properly kind: feature New feature or request problem: stale Issue has not been responded to in some time scope: cache Related to the cache scope: docs Documentation could be improved. Or changes that only affect docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cache is not cleared when transformer changes
2 participants
@oscard0m @agilgur5