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

Use patch-package to fix max-depth issue in react-native (temporary) #1762

Merged
merged 4 commits into from
Jan 21, 2020

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented Jan 8, 2020

Fixes #1747

There is an issue on iOS with nested views.

Searching for the ancestor view while measuring a shadow view layout relative to the ancestor is limited to 30. I couldn't find the reason nor commit where it was introduced (because I have no access to internal facebook commits).
I think we should keep looking for an ancestor view until the shadow view has the superview property.
facebook/react-native#26986

This is fixed here facebook/react-native#26986
It's merged to master and will land in 0.62 version of react-native, however it is a blocker for us since we can not nest more than 3 group blocks because of that. I know there is a PR with updating react-native to 0.61 but it will not solve that issue since the fix is not there yet. We could cherry-pick this commit to our fork but as I can see in this PR #1450 we will not use our fork anymore.

That's why I created this PR - to handle this on gutenberg-mobile repo level with patch-package.

The mechanism is very simple. I created a patch that is in the patches directory and contains my changes. The patch is applied in the prepare hook, so it doesn't require any additional steps.
So the flow looks like:

  • install dependencies
  • apply all patches from patches directory (it's done in prepare hook)

More info about patch-package - https://callstack.com/blog/say-goodbye-to-old-fashioned-forks-thanks-to-the-patch-package/

To test:

  • run yarn install
  • run yarn ios
  • add at least 4 nested group blocks
  • the app shouldn't crash

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@pinarol
Copy link
Contributor

pinarol commented Jan 9, 2020

Aha, didn't know we aren't going to use our fork anymore. I haven't tested this yet but can't think of any reasons to not to try. cc @koke

I have a question. Are we going to need to update our patch file name "react-native+0.60.0-patched.patch" as "react-native+0.61.2-patched.patch" ? If so, I suggest doing that in the existing PR once we merge this one.

@dratwas
Copy link
Contributor Author

dratwas commented Jan 9, 2020

I have a question. Are we going to need to update our patch file name "react-native+0.60.0-patched.patch" as "react-native+0.61.2-patched.patch" ? If so, I suggest doing that in the existing PR once we merge this one.

Yes we would need to create a new patch. But it is really easy, just run command patch-package react-native

@koke
Copy link
Member

koke commented Jan 9, 2020

Aha, didn't know we aren't going to use our fork anymore. I haven't tested this yet but can't think of any reasons to not to try. cc @koke

I haven't tested this, but it looks like a good approach to me 👌

@pinarol pinarol mentioned this pull request Jan 10, 2020
1 task
@pinarol
Copy link
Contributor

pinarol commented Jan 13, 2020

It looks like there are still things to do for 0.61. Since applying the patch is simple we can get this merged without needing to wait more. And we can re-apply the patch after that 0.61 is merged. Or directly into that PR. @koke wdyt? would you be able to review this one if that's ok for you too?

Copy link
Member

@koke koke left a comment

Choose a reason for hiding this comment

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

I think the PR looks fine, but I wasn't able to test because when I try to add a group block, I get a red screen:

Simulator Screen Shot - iPhone 11 - 2020-01-14 at 11 36 07

package.json Show resolved Hide resolved
@pinarol
Copy link
Contributor

pinarol commented Jan 14, 2020

That crash was fixed on the release branch with this PR.

@pinarol
Copy link
Contributor

pinarol commented Jan 16, 2020

Piotr 👋 Could you update the pr’s branch so the fix is included? Thanks!

@dratwas
Copy link
Contributor Author

dratwas commented Jan 16, 2020

@pinarol, @koke Done :)

Copy link
Member

@koke koke left a comment

Choose a reason for hiding this comment

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

I was able to test it and it works great 🎉

I noticed something that I don't think are caused by this PR. I added several levels of nested groups, and each level adds a bit of vertical padding to the placeholder:

Simulator Screen Shot - iPhone 11 - 2020-01-20 at 11 01 23

@jbinda
Copy link
Contributor

jbinda commented Jan 21, 2020

I noticed something that I don't think are caused by this PR. I added several levels of nested groups, and each level adds a bit of vertical padding to the placeholder:

@koke I think the issue is in the bordering logic. Adding condition which eliminates top margin in second child of selected item should fix that behaviour.

@pinarol
Copy link
Contributor

pinarol commented Jan 21, 2020

I think the issue is in the bordering logic. Adding condition which eliminates top margin in second child of selected item should fix that behaviour.

@jbinda could you open an issue for that and add to callstack board? Thanks in advance.

@jbinda
Copy link
Contributor

jbinda commented Jan 21, 2020

@jbinda could you open an issue for that and add to callstack board? Thanks in advance.

Sure

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.

Cherry pick "max depth" fix on our react-native fork
4 participants