Skip to content

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

Merged
dratwas merged 4 commits intodevelopfrom
fix/max-depth-patch-package
Jan 21, 2020
Merged

Use patch-package to fix max-depth issue in react-native (temporary)#1762
dratwas merged 4 commits intodevelopfrom
fix/max-depth-patch-package

Conversation

@dratwas
Copy link
Copy Markdown
Contributor

@dratwas dratwas commented Jan 8, 2020 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

dratwas commented Jan 9, 2020 •
edited
Loading

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
Copy Markdown
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
Merged
1 task
@pinarol
Copy link
Copy Markdown
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?

koke
koke reviewed Jan 14, 2020
View reviewed changes
Copy link
Copy Markdown
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

@pinarol
Copy link
Copy Markdown
Contributor

pinarol commented Jan 14, 2020

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

@pinarol
Copy link
Copy Markdown
Contributor

pinarol commented Jan 16, 2020

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

@dratwas
Copy link
Copy Markdown
Contributor Author

dratwas commented Jan 16, 2020

@pinarol, @koke Done :)

koke
koke approved these changes Jan 20, 2020
View reviewed changes
Copy link
Copy Markdown
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

@pinarol pinarol mentioned this pull request Jan 20, 2020
Closed
@jbinda
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

jbinda commented Jan 21, 2020

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

Sure

@dratwas dratwas merged commit 5276ed2 into develop Jan 21, 2020
@dratwas dratwas deleted the fix/max-depth-patch-package branch January 21, 2020 11:13
@jbinda jbinda mentioned this pull request Jan 21, 2020
Closed
@hypest hypest mentioned this pull request Jan 27, 2020
Open
@pinarol pinarol mentioned this pull request Jan 29, 2020
Closed
3 tasks
This was referenced Feb 3, 2020
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koke koke koke approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@dratwas @pinarol @koke @jbinda