-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
`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 Report
@@ Coverage Diff @@
## master #654 +/- ##
=======================================
Coverage 95.23% 95.23%
=======================================
Files 1 1
Lines 21 21
Branches 2 2
=======================================
Hits 20 20
Misses 1 1
Continue to review full report at Codecov.
|
@DylanVann this is minor but will help a lot! |
#676 is the same but with derived typing from the declared const |
@krisztiaan-uic sure, thanks a lot! I'll do it on weekends and post and update here |
Hi @krisztiaan-uic I've merged #676 code into this branch, so now However, all the recent CI checks fail due to Node v13 bug (Circle CI is configured to use |
CircleCI fix is ready at #683 |
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. |
Hi @DylanVann, The best thing is to have a new release :) Thank you for your time! |
Cache
property type was equal toPriority
(withlow | normal | high
values)However,
FastImage.cacheControl
containsimmutable
,web
andcacheOnly
values, so the types are not matchingFixed by changing
Cache
type to'immutable' | 'web' | 'cacheOnly'