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

Load assets from same folder as JSbundle (Android) #4527

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@geof90
Contributor

geof90 commented Dec 3, 2015

#3679 was only partially fixed as the behaviour only works on iOS. This implements the same behaviour for Android. If the JSBundle was loaded from the assets folder, this will load images from the built-in resources. Else, load the image from the same folder as the JS bundle.

EDIT: For added clarity:

On iOS,
Bundle Location: 'file:///Path/To/Sample.app/main.bundle'
httpServerLocation: '/assets/module/a/'
Name: 'logo'
type: 'png'
Resolved Asset location: '/Path/To/Sample.app/assets/module/a/logo.png'

On Android,
Bundle Location: 'file:///sdcard/Path/To/main.bundle'
httpServerLocation: '/assets/module/a/',
name: 'logo'
type: 'png'
Resolved Asset location: 'file:///sdcard/Path/To/drawable_mdpi/module_a_logo.png'

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 3, 2015

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

facebook-github-bot commented Dec 3, 2015

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

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 3, 2015

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

facebook-github-bot commented Dec 3, 2015

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 3, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 3, 2015

@geof90 updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 3, 2015

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot commented Dec 3, 2015

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 3, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 3, 2015

@geof90 updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 3, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 3, 2015

@geof90 updated the pull request.

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 3, 2015

Collaborator

In my app, the assets are bundled in the APK, while I update the JavaScript bundle only. I presume this will break the behaviour?

Collaborator

satya164 commented Dec 3, 2015

In my app, the assets are bundled in the APK, while I update the JavaScript bundle only. I presume this will break the behaviour?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Dec 3, 2015

Contributor

What do you mean by "load images from the same folder as the JS bundle"?

Contributor

mkonicek commented Dec 3, 2015

What do you mean by "load images from the same folder as the JS bundle"?

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 3, 2015

Collaborator

@mkonicek I guess it means that if I download the js bundle to a folder (say cache folder/sdcard folder), it'll load images from there.

Collaborator

satya164 commented Dec 3, 2015

@mkonicek I guess it means that if I download the js bundle to a folder (say cache folder/sdcard folder), it'll load images from there.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Dec 3, 2015

Contributor

Is this the same use case as in #3679? Do I understand it correctly you'd like to make resolution mechanism to be more configurable to be able to load those images from a folder on the device rather than from Android's AssetManager?
http://developer.android.com/reference/android/content/res/AssetManager.html

For that you'd need to change native code too, right?

How did you test this?

Contributor

mkonicek commented Dec 3, 2015

Is this the same use case as in #3679? Do I understand it correctly you'd like to make resolution mechanism to be more configurable to be able to load those images from a folder on the device rather than from Android's AssetManager?
http://developer.android.com/reference/android/content/res/AssetManager.html

For that you'd need to change native code too, right?

How did you test this?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Dec 3, 2015

Contributor

Also, there are unit tests for resolveAssetSource, please have a look at those.

Contributor

mkonicek commented Dec 3, 2015

Also, there are unit tests for resolveAssetSource, please have a look at those.

@geof90

This comment has been minimized.

Show comment
Hide comment
@geof90

geof90 Dec 3, 2015

Contributor

Hi @mkonicek yes indeed it is the same use case of enabling OTA updates to React Native apps. @frantic submitted a commit here 10b599c that enables the scenario for iOS. He did not make any change to native code in that commit, and I am pretty sure that the ReactImageView knows how to resolve file:// schemes to local filesystem files so there shouldn't be any need to change native code.

I manually tested it by loading a bundle from the local filesystem using ReactInstanceManager.builder().setJSBundleFile(), and verifying that the app displays images that resolve to local files after this change.

I will take a look at the unit tests, thanks.

Contributor

geof90 commented Dec 3, 2015

Hi @mkonicek yes indeed it is the same use case of enabling OTA updates to React Native apps. @frantic submitted a commit here 10b599c that enables the scenario for iOS. He did not make any change to native code in that commit, and I am pretty sure that the ReactImageView knows how to resolve file:// schemes to local filesystem files so there shouldn't be any need to change native code.

I manually tested it by loading a bundle from the local filesystem using ReactInstanceManager.builder().setJSBundleFile(), and verifying that the app displays images that resolve to local files after this change.

I will take a look at the unit tests, thanks.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 4, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 4, 2015

@geof90 updated the pull request.

@geof90

This comment has been minimized.

Show comment
Hide comment
@geof90

geof90 Dec 4, 2015

Contributor

@mkonicek I have updated the unit test. The unit test didnt fail at all inititially with my change. I noted that the file:// scheme only appears in the NativeModules.SourceCode.scriptURL in iOS. In android, if the script is loaded from the local file system, it is simply set to /sdcard/path/to/file.bundle or /data/data/path/to/file.bundle.

Contributor

geof90 commented Dec 4, 2015

@mkonicek I have updated the unit test. The unit test didnt fail at all inititially with my change. I noted that the file:// scheme only appears in the NativeModules.SourceCode.scriptURL in iOS. In android, if the script is loaded from the local file system, it is simply set to /sdcard/path/to/file.bundle or /data/data/path/to/file.bundle.

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 4, 2015

Collaborator

Can the asset directory be customizable? In an app where assets rarely change, it would be useful to only update the bundle and not the assets. Specifying a custom asset path can save unenecessary copying and all.

Collaborator

satya164 commented Dec 4, 2015

Can the asset directory be customizable? In an app where assets rarely change, it would be useful to only update the bundle and not the assets. Specifying a custom asset path can save unenecessary copying and all.

@geof90

This comment has been minimized.

Show comment
Hide comment
@geof90

geof90 Dec 4, 2015

Contributor

@satya164 is your scenario supported in iOS with this present build of React Native?

Contributor

geof90 commented Dec 4, 2015

@satya164 is your scenario supported in iOS with this present build of React Native?

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 4, 2015

Collaborator

@geof90 I'm only on Android ATM. But supporting this on iOS will also make sense, when I port my app to iOS.

I think there have been talks about being able to specify a custom image loader which can address all these kinds of use cases.

Collaborator

satya164 commented Dec 4, 2015

@geof90 I'm only on Android ATM. But supporting this on iOS will also make sense, when I port my app to iOS.

I think there have been talks about being able to specify a custom image loader which can address all these kinds of use cases.

@geof90

This comment has been minimized.

Show comment
Hide comment
@geof90

geof90 Dec 4, 2015

Contributor

@satya164 agreed. Right now, the asset resolution for Android always goes to the built-in resources, which is strange because the path given to require(..) is a relative path. This PR addresses that, achieve parity with the iOS side, and unblocking OTA asset + bundle updates. That said, a way to maybe pass in a flag so that assets will always be loaded from the binary resources regardless of where the bundle is loaded will be useful, and I would be interested if there was a good solution for that too since I am working on an app update service - https://github.com/Microsoft/react-native-code-push.

Contributor

geof90 commented Dec 4, 2015

@satya164 agreed. Right now, the asset resolution for Android always goes to the built-in resources, which is strange because the path given to require(..) is a relative path. This PR addresses that, achieve parity with the iOS side, and unblocking OTA asset + bundle updates. That said, a way to maybe pass in a flag so that assets will always be loaded from the binary resources regardless of where the bundle is loaded will be useful, and I would be interested if there was a good solution for that too since I am working on an app update service - https://github.com/Microsoft/react-native-code-push.

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 4, 2015

Collaborator

@geof90

Yeah, feature parity with iOS is the most voted item on Product Pains. Thanks a lot for working on this :D

Right now, the asset resolution for Android always goes to the built-in resources, which is strange because the path given to require(..) is a relative path

This is not entirely true. Right now, the bundle file is copied into the assets/ directory, while the images are copied to the res/ directory. So technically they are in different directories.

However, I would love to have the flag which I need to enable. I'm not sure what's the best way to do this though.

Thoughts @mkonicek @ide ?

Collaborator

satya164 commented Dec 4, 2015

@geof90

Yeah, feature parity with iOS is the most voted item on Product Pains. Thanks a lot for working on this :D

Right now, the asset resolution for Android always goes to the built-in resources, which is strange because the path given to require(..) is a relative path

This is not entirely true. Right now, the bundle file is copied into the assets/ directory, while the images are copied to the res/ directory. So technically they are in different directories.

However, I would love to have the flag which I need to enable. I'm not sure what's the best way to do this though.

Thoughts @mkonicek @ide ?

@zahanm

View changes

Show outdated Hide outdated Libraries/Image/resolveAssetSource.js
@zahanm

This comment has been minimized.

Show comment
Hide comment
@zahanm

zahanm Dec 4, 2015

Contributor

cc @natthu
you were speaking with @frantic about fresco being able to load file:// resources on Android, right?

Were you able to manually test this? edit: ah, yes you mentioned that you did.

Contributor

zahanm commented Dec 4, 2015

cc @natthu
you were speaking with @frantic about fresco being able to load file:// resources on Android, right?

Were you able to manually test this? edit: ah, yes you mentioned that you did.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 14, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 14, 2015

@geof90 updated the pull request.

@mkonicek

View changes

Show outdated Hide outdated local-cli/bundle/assetPathUtils.js
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 18, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 18, 2015

@geof90 updated the pull request.

@geof90

This comment has been minimized.

Show comment
Hide comment
@geof90

geof90 Dec 23, 2015

Contributor

Any updates on this? :)

Contributor

geof90 commented Dec 23, 2015

Any updates on this? :)

@frantic

View changes

Show outdated Hide outdated Libraries/Image/resolveAssetSource.js
@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Dec 23, 2015

Contributor

@geof90 looks good, thank you! Just a few nits. Meanwhile I'll run this on our internal test infra

Contributor

frantic commented Dec 23, 2015

@geof90 looks good, thank you! Just a few nits. Meanwhile I'll run this on our internal test infra

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic
Contributor

frantic commented Dec 23, 2015

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot commented Dec 23, 2015

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/968355239891442/int_phab to review.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 24, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 24, 2015

@geof90 updated the pull request.

@geof90

This comment has been minimized.

Show comment
Hide comment
@geof90

geof90 Dec 24, 2015

Contributor

Thanks for the reviews! Am excited to see this change go in.

Contributor

geof90 commented Dec 24, 2015

Thanks for the reviews! Am excited to see this change go in.

@lostintangent lostintangent referenced this pull request Dec 28, 2015

Closed

Remove custom dialog #117

@lostintangent

This comment has been minimized.

Show comment
Hide comment
@lostintangent

lostintangent Dec 28, 2015

@frantic How did the test run go? Does this PR still need a revision or can that tag be removed?

lostintangent commented Dec 28, 2015

@frantic How did the test run go? Does this PR still need a revision or can that tag be removed?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 28, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 28, 2015

@geof90 updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 29, 2015

@geof90 updated the pull request.

facebook-github-bot commented Dec 29, 2015

@geof90 updated the pull request.

@tlvenn

This comment has been minimized.

Show comment
Hide comment
@tlvenn

tlvenn Jan 5, 2016

Contributor

Any chance to move forward with this PR ? It would be awesome to have it lands for RN 0.18

Contributor

tlvenn commented Jan 5, 2016

Any chance to move forward with this PR ? It would be awesome to have it lands for RN 0.18

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Jan 5, 2016

Contributor

I'm working on it, sorry for the wait guys.

Contributor

frantic commented Jan 5, 2016

I'm working on it, sorry for the wait guys.

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic
Contributor

frantic commented Jan 5, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot commented Jan 5, 2016

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/968355239891442/int_phab to review.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 5, 2016

Contributor

Wohoo!

Contributor

mkonicek commented Jan 5, 2016

Wohoo!

@lostintangent

This comment has been minimized.

Show comment
Hide comment
@lostintangent

lostintangent Jan 5, 2016

Yay! Is there any chance for this to make it into the final 0.18 release? :D

lostintangent commented Jan 5, 2016

Yay! Is there any chance for this to make it into the final 0.18 release? :D

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Jan 6, 2016

Collaborator

I guess not. This is a breaking change.

Collaborator

satya164 commented Jan 6, 2016

I guess not. This is a breaking change.

@ghost ghost closed this in e730a9f Jan 6, 2016

christopherdro added a commit to wildlifela/react-native that referenced this pull request Jan 20, 2016

Load assets from same folder as JSbundle (Android)
Summary:
facebook#3679 was only partially fixed as the behaviour only works on iOS. This implements the same behaviour for Android. If the JSBundle was loaded from the assets folder, this will load images from the built-in resources. Else, load the image from the same folder as the JS bundle.

EDIT: For added clarity:

On iOS,
Bundle Location: 'file:///Path/To/Sample.app/main.bundle'
httpServerLocation: '/assets/module/a/'
Name: 'logo'
type: 'png'
**Resolved Asset location: '/Path/To/Sample.app/assets/module/a/logo.png'**

On Android,
Bundle Location: 'file:///sdcard/Path/To/main.bundle'
httpServerLocation: '/assets/module/a/',
name: 'logo'
type: 'png'
**Resolved Asset location: 'file:///sdcard/Path/To/drawable_mdpi/module_a_logo.png'**
Closes facebook#4527

Reviewed By: svcscm

Differential Revision: D2788005

Pulled By: mkonicek

fb-gh-sync-id: 3f6462a7ee6370a92dd6727ac422c5de346c3ff1

grabbou pushed a commit to react-native-community/react-native-cli that referenced this pull request Sep 26, 2018

Load assets from same folder as JSbundle (Android)
Summary:
facebook/react-native#3679 was only partially fixed as the behaviour only works on iOS. This implements the same behaviour for Android. If the JSBundle was loaded from the assets folder, this will load images from the built-in resources. Else, load the image from the same folder as the JS bundle.

EDIT: For added clarity:

On iOS,
Bundle Location: 'file:///Path/To/Sample.app/main.bundle'
httpServerLocation: '/assets/module/a/'
Name: 'logo'
type: 'png'
**Resolved Asset location: '/Path/To/Sample.app/assets/module/a/logo.png'**

On Android,
Bundle Location: 'file:///sdcard/Path/To/main.bundle'
httpServerLocation: '/assets/module/a/',
name: 'logo'
type: 'png'
**Resolved Asset location: 'file:///sdcard/Path/To/drawable_mdpi/module_a_logo.png'**
Closes facebook/react-native#4527

Reviewed By: svcscm

Differential Revision: D2788005

Pulled By: mkonicek

fb-gh-sync-id: 3f6462a7ee6370a92dd6727ac422c5de346c3ff1

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment