Caching assets only for production environment #10919
Conversation
|
By analyzing the blame information on this pull request, we identified @davidaurelio and @frantic to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
I find it to be a significant source of subtle bugs when development and production modes differ, especially for something as important as caching. We should either always cache or never cache. Perhaps it make sense to expose an explicit flag for that but it should be independent of dev vs. production. In this case, the asset URL uses content addressing so I'd expect that modifying your assets will always bust the cache. And could you clear the cache on the device instead when needed? |
Modifying the asset URL will bust the cache but not the content. So, I've been doing stuff like "src='js/main.js?version=123'" to see changes. Clearing the cache on the device might work but would be fairly cumbersome to do on every change & reload cycle. A cacheControl propType (number) for the webview seems reasonable? |
There was a problem hiding this comment.
cc @davidaurelio, this looks like a reasonable change. Can you predict any breakage this would cause? Could it potentially slow down development build times?
| @@ -379,7 +379,7 @@ describe('processRequest', () => { | |||
|
|
|||
| server.processRequest(req, res); | |||
| jest.runAllTimers(); | |||
| expect(res.setHeader).toBeCalledWith('Cache-Control', 'max-age=31536000'); | |||
| // expect(res.setHeader).toBeCalledWith('Cache-Control', 'max-age=31536000'); | |||
There was a problem hiding this comment.
Lets avoid commenting out lines and leaving them in here. Can we just remove them instead?
|
Can you change this to another env var ex: REACT_NATIVE_ENABLE_ASSET_CACHING? |
grabcode
force-pushed
the
fix/caching-webview-asset-on-prod-only
branch
from
9ca893f
to
ada7912
Compare
|
All done. Thanks for reviewing so quickly. |
| @@ -502,7 +502,9 @@ class Server { | |||
| data => { | |||
| // Tell clients to cache this for 1 year. | |||
| // This is safe as the asset url contains a hash of the asset. | |||
There was a problem hiding this comment.
Is this comment not true any longer, or is it only referring to the urls of require('d assets? Either way, we should update this comment and explain the intention. Not every app serves to WebViews so people may forget why this is the default.
|
This change seems reasonable and I wonder why we had the caching in the first place. I pinged @davidaurelio on messenger to ask before we merge this. |
|
lgtm |
|
@davidaurelio has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Thanks @davidaurelio & @mkonicek. Should I delete the branch? |
|
Quick question: why do we need an env var to bring this behavior back? I'd prefer to limit the number of obscure env variables we support. |
|
Good point, it is obscure. The term 'production' is used here in a misleading way. RNP is never serving prod- only for development and production builds. I would be in support of removing this caching feature entirely. Does anybody have a use case for it? |
|
@ericvicenti presumably there should be a small latency improvement when caching assets, no? The flag could be useful for production builds still. |
|
I'm still a little confused about why we couldn't always enable caching since the asset URL contained a hash of the asset. That is, there should never be stale or incorrect caches with that approach. |
|
I'm the one who added the cache header in the first place. This was to fix local images being reloaded from the packager everytime they were displayed. I think the problem here is the packager not recomputing the hash and hence not providing a new url when the asset content actually changes. |
Summary: **Motivation** In the context of a webview, one may extract assets (javascript or any types really), and relates to them through the html. The packager `Server` serves this files correctly but also applies a cache based on time (a year). During development, this cache is actually bad as we need to re-iterate the process of editing/testing quickly. I don't believe it is necessary to cache, and still wanted to make sure we would if process.env.NODE_ENV is 'production'. **Test plan** Run jest on impacted files: ``` node_modules/.bin/jest packager/react-packager/src/Server/__tests__/Server-test.js ``` Closes facebook#10919 Differential Revision: D4226350 Pulled By: davidaurelio fbshipit-source-id: d4bbff5b1a5b691aab197bcddb8fa9d2e43caa16
Summary: **Motivation** In the context of a webview, one may extract assets (javascript or any types really), and relates to them through the html. The packager `Server` serves this files correctly but also applies a cache based on time (a year). During development, this cache is actually bad as we need to re-iterate the process of editing/testing quickly. I don't believe it is necessary to cache, and still wanted to make sure we would if process.env.NODE_ENV is 'production'. **Test plan** Run jest on impacted files: ``` node_modules/.bin/jest packager/react-packager/src/Server/__tests__/Server-test.js ``` Closes facebook/react-native#10919 Differential Revision: D4226350 Pulled By: davidaurelio fbshipit-source-id: d4bbff5b1a5b691aab197bcddb8fa9d2e43caa16
Motivation
In the context of a webview, one may extract assets (javascript or any types really), and relates to them through the html.
The packager
Serverserves this files correctly but also applies a cache based on time (a year). During development, this cache is actually bad as we need to re-iterate the process of editing/testing quickly.I don't believe it is necessary to cache, and still wanted to make sure we would if process.env.NODE_ENV is 'production'.
Test plan
Run jest on impacted files: