🔥 Selects firebase.json file from wrong project

simonbengtsson commented 4 years ago

Issue

If you don't create a firebase.json file for a project the build phase script tries to find one in other directories. Since it searches parent folders it is highly likely it will find other projects which might have a firebase.json file. This means it applies options unrelated to current project. An example can be seen from our latest build:

info: -> RNFB build script started
info: 1) Locating firebase.json file:
info:      (1 of 2) Searching in '/Users/simon/workspace/equilab-react-native' for a firebase.json file.
info:      (2 of 2) Searching in '/Users/simon/workspace' for a firebase.json file.
info:      firebase.json found at /Users/simon/workspace/ideabook/firebase.json
info: 2) Injecting Info.plist entries:

As can be seen in the above logs it selected the firebase.json file for the ideabook project even though the current project is equilab-react-native.

If I wrote a PR stopping the parent folder search, will it be accepted?

Project Files

N/A

Environment

Click To Expand

**`react-native info` output:** ``` info Fetching system and libraries information... System: OS: macOS 10.15.3 CPU: (8) x64 Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz Memory: 1.50 GB / 16.00 GB Shell: 5.7.1 - /bin/zsh Binaries: Node: 10.16.0 - ~/.nvm/versions/node/v10.16.0/bin/node npm: 6.10.0 - ~/.nvm/versions/node/v10.16.0/bin/npm SDKs: iOS SDK: Platforms: iOS 13.2, DriverKit 19.0, macOS 10.15, tvOS 13.2, watchOS 6.1 Android SDK: API Levels: 25, 26, 28, 29 Build Tools: 28.0.3, 29.0.0 System Images: android-16 | Google APIs Intel x86 Atom, android-23 | Google APIs Intel x86 Atom, android-24 | Google APIs Intel x86 Atom_64, android-26 | Google APIs Intel x86 Atom, android-28 | Google Play Intel x86 Atom, android-29 | Intel x86 Atom_64 IDEs: Android Studio: 3.5 AI-191.8026.42.35.6010548 Xcode: 11.3.1/11C504 - /usr/bin/xcodebuild npm Packages: react: 16.9.0 => 16.9.0 react-native: 0.61.5 => 0.61.5 ``` - **Platform that you're experiencing the issue on**: - [x] iOS - [ ] Android - [ ] **iOS** but have not tested behavior on Android - [ ] **Android** but have not tested behavior on iOS - [ ] Both - **`react-native-firebase` version you're using that has this issue:** - `6.3.4` - **`Firebase` module(s) you're using that has the issue:** - `app` - **Are you using `TypeScript`?** - `No. But hopefully soon!`

mikehardy commented 4 years ago

That's probably most unexpected! Seems like it should never descend a sibling directory in my simple opinion. Honestly I would expect it to look in one spot only by default, with an override to search only if desired

stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

simonbengtsson commented 4 years ago

@mikehardy Should I submit a PR?

mikehardy commented 4 years ago

@Salakar what do you think? I personally would prefer for no searching but a big fat fail-fast error would be good. At minimum I don't think it should look in sibling directories, but this is all a taste issue.

stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

Salakar commented 4 years ago

This one slipped by me sorry.

The reason for the upwards searching is for monorepo support, though it definitely shouldn't be going up and then down into a different directory like it is here - so this will need fixing

stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

simonbengtsson commented 4 years ago

As far as I know it has not been addressed. I'm happy to attempt a fix for this if given okay 👌

z366 commented 4 years ago

It would be nice if some one would fix it I would like to know when it gets done I might have had a hand in do it if so I I'll do better

mikehardy commented 4 years ago

The build items like that are something I'd coordinate with @Salakar on - it cuts across the entire project so I would wait (or be okay with just a local patch) unless he considers it and approves the direction

stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

simonbengtsson commented 4 years ago

As far as I know it has not been addressed. I'm happy to attempt a fix for this if given okay 👌

mikehardy commented 4 years ago

This is going to need more serious maintainer thought to make sure there are no unintended failures if the search space for firebase.json is constrained - @Salakar ?

mikehardy commented 4 years ago

@simonbengtsson sorry this has sat idle.

If you could propose a PR that:

continued to allow upward searching
disallowed upward-then-down-to-some-sibling-directory

I believe that would be acceptable and easy to merge+release, a non-breaking change.

I personally find the upward searching even problematic but it would be a breaking change to alter that. I'm not sure where it would need to be implemented but if there were a way to set a toggle that controlled where searching happened (a gradle variable? a podfile variable?) that turned it on or off completely then we could at least support the current monorepo use case - possibly by adding documentation and issuing a breaking change where it defaults to off but people could opt-in if desired

What do you think?

Cross-referencing #3902

simonbengtsson commented 4 years ago

Sounds good to me! I'll try to submit one soon that at least disallow upward-then-down-to-some-sibling-directory search.

On Mon, 31 Aug 2020 at 17:34, Mike Hardy notifications@github.com wrote:

@simonbengtsson https://github.com/simonbengtsson sorry this has sat idle.

If you could propose a PR that:

continued to allow upward searching

disallowed upward-then-down-to-some-sibling-directory

I believe that would be acceptable and easy to merge+release, a non-breaking change.

I personally find the upward searching even problematic but it would be a breaking change to alter that. I'm not sure where it would need to be implemented but if there were a way to set a toggle that controlled where searching happened (a gradle variable? a podfile variable?) that turned it on or off completely then we could at least support the current monorepo use case - possibly by adding documentation and issuing a breaking change where it defaults to off but people could opt-in if desired

What do you think?

Cross-referencing #3902 https://github.com/invertase/react-native-firebase/issues/3902

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/invertase/react-native-firebase/issues/3307#issuecomment-683854245, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3LVAZUDCFH7UZW7V2DQO3SDO7GTANCNFSM4LMC6ZQA .

stale[bot] commented 4 years ago

Hello 👋, to help manage issues we automatically close stale issues. This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 3 years ago

Closing this issue after a prolonged period of inactivity. If this is still present in the latest release, please feel free to create a new issue with up-to-date information.

invertase / react-native-firebase