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

fix: supporting browfields implementations with a differing react root #1174

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Debens
Copy link

@Debens Debens commented Oct 30, 2019

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: react-native-webview/react-native-webview@24ec4f7

@msand
Copy link
Collaborator

msand commented Nov 1, 2019

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

@msand
Copy link
Collaborator

msand commented Nov 3, 2019

@SaeedZhiany You have any feedback on this?

@SaeedZhiany
Copy link
Contributor

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?

@Debens
Copy link
Author

Debens commented Nov 4, 2019

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
Copy link
Author

Debens commented Nov 4, 2019

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

@msand
Copy link
Collaborator

msand commented Nov 4, 2019

@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.

@SaeedZhiany
Copy link
Contributor

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?

@Debens
Copy link
Author

Debens commented Nov 5, 2019

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
Copy link
Author

Debens commented Nov 5, 2019

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

@msand
Copy link
Collaborator

msand commented Nov 5, 2019

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

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.

3 participants