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

Fix Source's Cache type #654

Closed
wants to merge 3 commits into from
Closed

Fix Source's Cache type #654

wants to merge 3 commits into from

Conversation

kulyk
Copy link

@kulyk kulyk commented Mar 22, 2020

Cache property type was equal to Priority (with low | normal | high values)
However, FastImage.cacheControl contains immutable, web and cacheOnly values, so the types are not matching

Fixed by changing Cache type to 'immutable' | 'web' | 'cacheOnly'

`Cache` property type was equal to `Priority` (with `low | normal | high` values)
However, `FastImage.cacheControl` contains `immutable`, `web` and `cacheOnly` values, so the types are not matching

Fixed by changing `Cache` type to `'immutable' | 'web' | 'cacheOnly'`
@codecov
Copy link

codecov bot commented Mar 22, 2020

Codecov Report

Merging #654 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #654   +/-   ##
=======================================
  Coverage   95.23%   95.23%           
=======================================
  Files           1        1           
  Lines          21       21           
  Branches        2        2           
=======================================
  Hits           20       20           
  Misses          1        1           
Impacted Files Coverage Δ
src/index.tsx 95.23% <ø> (ø)

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 cde7af5...8b82228. Read the comment docs.

@radko93
Copy link

radko93 commented Apr 29, 2020

@DylanVann this is minor but will help a lot!

@krisztiaan-uic
Copy link

#676 is the same but with derived typing from the declared const

@krisztiaan-uic
Copy link

krisztiaan-uic commented May 14, 2020

@kulyk would you mind merging #676 into yours, so I could close that one?

also, anyone can just make these changes locally, and use patch-package to apply it in your current project until this one is merged

@kulyk
Copy link
Author

kulyk commented May 14, 2020

@krisztiaan-uic sure, thanks a lot! I'll do it on weekends and post and update here

@kulyk
Copy link
Author

kulyk commented May 17, 2020

Hi @krisztiaan-uic I've merged #676 code into this branch, so now Priority and Cache types are derived from const values and exported. Thank you for your work!

However, all the recent CI checks fail due to Node v13 bug (Circle CI is configured to use node:latest image). I'll open a new PR to fix the CI checks a bit later

@kulyk
Copy link
Author

kulyk commented May 17, 2020

CircleCI fix is ready at #683
@DylanVann hi, please take a look 🙏

@DylanVann
Copy link
Owner

Hi @kulyk, thanks for the PR!

I merged the other one to fix the Cache type. I think it would be better to just use string unions rather than exporting enum like objects, then this issue won't end up reoccurring.

@DylanVann DylanVann closed this Jul 17, 2020
@kulyk
Copy link
Author

kulyk commented Jul 17, 2020

Hi @DylanVann,

The best thing is to have a new release :) Thank you for your time!

@kulyk kulyk deleted the patch-1 branch July 17, 2020 09:05
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.

4 participants