Fix iOS unchanged local assets refetched from packager in development #9795

jeanregisser · 2016-09-07T17:07:41Z

Hi,

This PR fixes the problem described by @chrisnojima in #9581 (comment)

Test plan

In development mode,

Run an app with an image: <Image source={ require('./logo.png') }/>
Notice that you see the following in packager console:

6:46:42 PM] <START> processing asset request logo.png
[6:46:42 PM] <END>   processing asset request logo.png (1ms)

Reload the app, or navigate to another page of the app with the same image
Notice that you see again:

6:47:23 PM] <START> processing asset request logo.png
[6:47:23 PM] <END>   processing asset request logo.png (1ms)

Now wih the fix applied,
notice that you only see logo.png fetched once, even if you reload or show the same image in a different part of the app.

Let me know what you think.

ghost · 2016-09-07T17:08:10Z

By analyzing the blame information on this pull request, we identified @davidaurelio and @frantic to be potential reviewers.

jeanregisser · 2016-09-07T17:08:43Z

Also see my comment #9581 (comment) which explains what caused the issue.

mkonicek · 2016-09-08T13:29:11Z

@jeanregisser I agree #9581 is a pretty important issue - in production images should be cached and not always refetched.

Why not always refetch them from the packager during development though?

With the change in this PR, what if I update the logo.png in the filesystem and Reload JS? The app should render the new logo immediately. Will it?

jeanregisser · 2016-09-08T13:59:33Z

I think it's unexpected to always refetch the same local assets in development mode.

For instance, if you have a screen showing 10 local images, the first time it's displayed, they are going to be fetched from the packager and this is normal and expected.
Now if you have another screen using the same local images, it's unexpected that you don't see them immediately as they are refetched again when you navigate to it.

Up until RN < 0.31.0 local images were just fetched once (unless you changed them).

The PR restores this behavior on iOS. And if you change logo.png and reload, you'll see the new logo as expected.
On Android it doesn't change anything. It was already behaving that way.

jeanregisser · 2016-09-16T13:35:00Z

@mkonicek what do you think of my answer?

davidaurelio · 2016-09-16T15:39:26Z

Thanks for repairing this!

davidaurelio · 2016-09-16T15:39:37Z

@facebook-github-bot shipit

facebook-github-bot · 2016-09-16T15:40:40Z

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

davidaurelio · 2016-09-16T16:22:06Z

@jeanregisser there are a couple of tests failing. Could you fix or adapt them, please?

jeanregisser · 2016-09-16T16:23:47Z

Yes. I'll have a look.

ghost · 2016-09-17T05:48:09Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

ide · 2016-09-17T06:19:07Z

packager/react-packager/src/Server/index.js

-        data => res.end(this._rangeRequestMiddleware(req, res, data, assetPath)),
+        data => {
+          // Tell clients to cache this for 1 year.
+          // This is safe as the asset url contains a hash of the mtime of the asset.


Should this say hash of the asset files instead of mtime now?

Good catch!

Updated thanks.

ghost · 2016-09-19T09:17:39Z

@jeanregisser updated the pull request - view changes - changes since last import

Summary: Hi, This PR fixes the problem described by chrisnojima in facebook/react-native#9581 (comment) **Test plan** In development mode, - Run an app with an image: `<Image source={ require('./logo.png') }/>` - Notice that you see the following in packager console: ```txt 6:46:42 PM] <START> processing asset request logo.png [6:46:42 PM] <END> processing asset request logo.png (1ms) ``` - Reload the app, or navigate to another page of the app with the same image - Notice that you see again: ```txt 6:47:23 PM] <START> processing asset request logo.png [6:47:23 PM] <END> processing asset request logo.png (1ms) ``` Now wih the fix applied, notice that you only see `logo.png` fetched once, even if you reload or show the same image in a different part of the app. Let me know what you think. Closes facebook/react-native#9795 Differential Revision: D3876945 Pulled By: davidaurelio fbshipit-source-id: f41f4719e87644692a690123fd6e54eead9cc87d

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 7, 2016

mkonicek added the Needs more information label Sep 8, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 8, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 16, 2016

ghost added Import Failed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Sep 17, 2016

ide reviewed Sep 17, 2016

View reviewed changes

Fix iOS local assets refetched from packager in development

f272ff2

jeanregisser force-pushed the fix-local-assets-refetched-in-development branch from c0ba6c8 to f272ff2 Compare September 19, 2016 09:16

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 19, 2016

ghost closed this in 1a3e6eb Sep 19, 2016

bestander mentioned this pull request Sep 20, 2016

[0.32] Image caching broken #9581

Closed

jeanregisser mentioned this pull request Nov 29, 2016

Caching assets only for production environment #10919

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix iOS unchanged local assets refetched from packager in development #9795

Fix iOS unchanged local assets refetched from packager in development #9795

jeanregisser commented Sep 7, 2016 •

edited

Loading

ghost commented Sep 7, 2016

jeanregisser commented Sep 7, 2016

mkonicek commented Sep 8, 2016

jeanregisser commented Sep 8, 2016

jeanregisser commented Sep 16, 2016

davidaurelio commented Sep 16, 2016

davidaurelio commented Sep 16, 2016

facebook-github-bot commented Sep 16, 2016

davidaurelio commented Sep 16, 2016

jeanregisser commented Sep 16, 2016

ghost commented Sep 17, 2016

ide Sep 17, 2016

davidaurelio Sep 17, 2016

jeanregisser Sep 19, 2016

ghost commented Sep 19, 2016

Fix iOS unchanged local assets refetched from packager in development #9795

Fix iOS unchanged local assets refetched from packager in development #9795

Conversation

jeanregisser commented Sep 7, 2016 • edited Loading

ghost commented Sep 7, 2016

jeanregisser commented Sep 7, 2016

mkonicek commented Sep 8, 2016

jeanregisser commented Sep 8, 2016

jeanregisser commented Sep 16, 2016

davidaurelio commented Sep 16, 2016

davidaurelio commented Sep 16, 2016

facebook-github-bot commented Sep 16, 2016

davidaurelio commented Sep 16, 2016

jeanregisser commented Sep 16, 2016

ghost commented Sep 17, 2016

ide Sep 17, 2016

Choose a reason for hiding this comment

davidaurelio Sep 17, 2016

Choose a reason for hiding this comment

jeanregisser Sep 19, 2016

Choose a reason for hiding this comment

ghost commented Sep 19, 2016

jeanregisser commented Sep 7, 2016 •

edited

Loading