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

Migrating to AndroidX #500

Closed
wants to merge 9 commits into from
Closed

Conversation

vokhuyetOz
Copy link

No description provided.

@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #500   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files           1        1           
  Lines          19       19           
=======================================
  Hits           18       18           
  Misses          1        1

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 7f080bf...40ec7e9. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #500   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files           1        1           
  Lines          19       19           
=======================================
  Hits           18       18           
  Misses          1        1

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 df76905...d81f856. Read the comment docs.

@WrathChaos
Copy link

Waiting for this PR...

@mkamals
Copy link

mkamals commented Jun 24, 2019

Kindly merge this PR soon and release a new version of the package. This is creating lot of issues with the build generation in Android.

@arunalt
Copy link

arunalt commented Jun 24, 2019

Hey @DylanVann,

Do we have any ETA on when this PR will get merged? We're looking to migrate to AndroidX and this is a huge dependency for us. Would really appreciate a response!

Cheers.
Arun.

@rborn
Copy link

rborn commented Jun 25, 2019

@DylanVann I can confirm the PR works well (I'm using it now as replacement for this one till it gets merged)
@vokhuyetOz maybe you could rebase so the PR can be merged when the time comes ? ❤️

@die20
Copy link

die20 commented Jun 26, 2019

For some reason when I try to fork this branch and try to use it in an application, I get this when I run yarn install:

fatal: not a git repository (or any of the parent directories): .git
error Command failed with exit code 128.

This is how I'm trying to use it on the package.json:

"react-native-fast-image": "git+https://github.com/vokhuyetOz/react-native-fast-image",

@rborn
Copy link

rborn commented Jun 27, 2019

@die20
"react-native-fast-image": "git+https://github.com/vokhuyetOz/react-native-fast-image.git",

you forgot the trailing .git 😊

@JimtotheB
Copy link

JimtotheB commented Jun 27, 2019

@rborn The trailing .git shouldn't matter, I can reproduce this behavior in a bare project, It also produced the same error with yarn add https://github.com/DylanVann/react-native-fast-image which is 100% the correct way to add a git repo via yarn

{
  "name": "fake",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "license": "ISC",
  "dependencies": {
    "lodash": "git+https://github.com/lodash/lodash"
  }

This works just fine. 😊

@rborn
Copy link

rborn commented Jun 27, 2019

@JimtotheB this works for me, however it takes 1000 years to finish 😔

npm install -ddd https://github.com/DylanVann/react-native-fast-image.git

@JimtotheB
Copy link

@rborn, I think this is specific to yarn, I'm sure npm is unpacking the submodule correctly and yarn is not.

@die20
Copy link

die20 commented Jun 27, 2019

@rborn It seems to be a yarn issue. It's not just this branch though, it's the master of react-native-fast-image.

I was able to get it installed with npm but remote gifs appear to not be working with this commit. The gif does not play.

@Ilphrin
Copy link

Ilphrin commented Jul 4, 2019

Thanks for this PR =D
There will be a need for an approval now that RN 60 is out, it needs AndroidX support for all libraries so until this PR is merged, react-native-fast-image may not work anymore on Android for 0.60 :/ We do not use this version yet so can't test yet though...

@DylanVann
Copy link
Owner

I have tried this PR, it does work with the new RN version by default.

The one thing blocking me from merging is that the example application includes other 3rd party libraries that do not support AndroidX.

@vokhuyetOz
Copy link
Author

@DylanVann i saw react-native-gesture-handle don't migrate to androidX, it causes the error

@vokhuyetOz
Copy link
Author

should we wait for it ?

@DylanVann
Copy link
Owner

DylanVann commented Jul 5, 2019

I used jetify to fix the examples. This is great, thanks @vokhuyetOz! I ended up merging from another branch.

@DylanVann DylanVann closed this Jul 5, 2019
@mkamals
Copy link

mkamals commented Jul 5, 2019

Thanks @DylanVann for merging this PR.

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.

9 participants