react-native-svg icon indicating copy to clipboard operation
react-native-svg copied to clipboard

fix: supporting browfields implementations with a differing react root

Open Debens opened this issue 6 years ago • 10 comments

Summary

With the change found here react-native-svg lost support for an implementation where any node dependencies are not located specifically at the root of the project.

This PR borrows the gradle implementation from react-native-webview that does a better job a resolving the react root of a project and warns when this cannot be resolved.

See: https://github.com/react-native-community/react-native-webview/commit/24ec4f752cdfd29d49b0a13a0035aaea736eb641

Debens avatar Oct 30 '19 12:10 Debens

@Debens Thanks for this! What kinds of testing have you done to verify this works correctly?

msand avatar Nov 01 '19 09:11 msand

@SaeedZhiany You have any feedback on this?

msand avatar Nov 03 '19 22:11 msand

Honestly, I didn't figure out what exactly the problem is. @Debens can you explain more? I never had such a problem (not to be able finding node dependencies) and I agree with @msand, what are the use cases that the problem occurs?

SaeedZhiany avatar Nov 04 '19 05:11 SaeedZhiany

Apologies for the delayed response. The issue here is that the current implementation relies on the node modules being in a specific location. A folder down from the android app.

While this is fine for a greenfields application made from something like react-native init in a brownfields implementation it is more common to have the react root in the root of what is an android project, "$rootDir/node_modules/", or even under a sub directory "$rootDir/react/node_modules/".

Debens avatar Nov 04 '19 10:11 Debens

Related to testing I haven't been able to get much done outside of a personal use case. If you give me some time I can throw a tester app together. But it might be a few days as I have other commitments currently

Debens avatar Nov 04 '19 10:11 Debens

@Debens You could try making a PR to https://github.com/msand/react-native-svg-e2e fork and checkout the project, yarn add the commit from your pr, commit and push to a new branch in your fork, and create a pr, then the end to end tests should run in travis and bitrise, producing screenshots if everything worked out.

msand avatar Nov 04 '19 10:11 msand

I understand now, I didn't experience integrating react-native with an existing android project. according to this page from react-native documentation (especially this part), the existing android project should be placed into a folder consisting package.json.

rootFolder
|____ android
     |____ content of the existing android project's folder
|____ package.json

So after installing the dependencies using npm install or yarn install the node_modules will be created in the rootFolder

rootFolder
|____ android
     |____ content of the existing android project's folder
|____ node_modules
|____ package.json

it seems this is the most common way for the integration process because it is documented.

So the current gradle implementation seems OK and no need for handling complicated cases for two reasons:

These changes

  1. add a new extra property reactNativeAndroidRoot to root react-native project that there is no naming convention on it, other library developers may expect this property with a different naming (like RNAndroidRootDir)
  2. allows the existing project to have a different structure than the normal react-native projects, while by the current implementation the users have been forced to respect the agreed react-native project structures.

So I suggest avoiding adding these changes.

@Debens Am I figure out the problem correctly at all? @msand what do you think?

SaeedZhiany avatar Nov 04 '19 10:11 SaeedZhiany

Yes you seems to have it! Sadly our integration into our existing app was not the smoothest, and while me agreed it was the right decision the native apps wanted it to be invasive as possible. We are only a single feature within a much larger app and it seems unreasonable to reorganise an already proven structure for a single important feature.

It is not necessary to use the reactNativeAndroidRoot property that ends being scoped to this project, this solution can find the root without one. It is only optional and follows the convention set out by other community projects that support a moveable react root in relation to the documentation you have provided. It might be preferable to switch instead to looking for and using the default react-native config react extension which also does support a different react root.

Debens avatar Nov 05 '19 09:11 Debens

@msand thanks for pointing me to the repo, here is a PR.

Debens avatar Nov 05 '19 10:11 Debens

The ci looks green with the react-native v0.60 setup now at least. Seems this should work out fine.

msand avatar Nov 05 '19 14:11 msand