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.

Sign up for GitHub

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

feat: improve monorepo support by removing redundant PROJECT_ROOT #28354

Closed
wants to merge 1 commit into from
Closed

feat: improve monorepo support by removing redundant PROJECT_ROOT #28354

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions scripts/react-native-xcode.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,17 @@ case "$CONFIGURATION" in
;;
esac

# Path to react-native folder inside node_modules
REACT_NATIVE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
# The project should be located next to where react-native is installed
# in node_modules.
PROJECT_ROOT=${PROJECT_ROOT:-"$REACT_NATIVE_DIR/../.."}
# Setting up a project root was a workaround to enable support for non-standard
# structures, including monorepos. Today, CLI supports that out of the box
# and setting custom `PROJECT_ROOT` only makes it confusing.
#
# As a backwards-compatible change, I am leaving "PROJECT_ROOT" support for those
# who already use it - it is likely a non-breaking removal.
#
# For new users, we default to $PWD - not changing things all.
#
# For context: https://github.com/facebook/react-native/commit/9ccde378b6e6379df61f9d968be6346ca6be7ead#commitcomment-37914902
PROJECT_ROOT=${PROJECT_ROOT:-$PWD}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this PR is old, but I believe we have a bug here. If $PWD is going to be the ios folder when this script is invoked from Xcode, then this line should be

PROJECT_ROOT=${PROJECT_ROOT:-$PWD/..}

cc @grabbou

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stigi would you mind opening a PR agains the main repo with the one line change you propose there? Once it's up it'd be way easier to test it & then have the fix you are proposing available for everyone :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kelset I was to quick to shoot. There's already a PR that is discussing the issue. It's not only PROJECT_ROOT that's broken, but also REACT_NATIVE_DIR. I left my comments in the PR #29477 (comment)


cd "$PROJECT_ROOT" || exit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this entirely, but I don't really want to run into all the users that have some crazy setup today. Historically, there was a lot of hacks around setting up monorepos and we're working with the CLI team to educate the developers that some of these are not needed anymore.


Expand Down Expand Up @@ -96,6 +102,9 @@ if [[ ! -x node && -d ${HOME}/.anyenv/bin ]]; then
fi
fi

# Path to react-native folder inside node_modules
REACT_NATIVE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"

# check and assign NODE_BINARY env
# shellcheck source=/dev/null
source "$REACT_NATIVE_DIR/scripts/node-binary.sh"
Expand Down