Skip to content

[RNMobile] Fix React Native error from function declaration hoisting #29013

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

Jump to bottom
Merged
merged 1 commit into from
Feb 16, 2021
Merged

[RNMobile] Fix React Native error from function declaration hoisting #29013

merged 1 commit into from
Feb 16, 2021

Conversation

dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented Feb 15, 2021 •
edited
Loading

Gutenberg Mobile: wordpress-mobile/gutenberg-mobile#3150

Description

Fixes wordpress-mobile/gutenberg-mobile#3149.

It is unknown why relying upon function declaration hoisting causes an
error in React Native, but it does occur. Recent changes with #28676
refactored a class to a functional component, including leveraging
function declaration hoisting. I.e. the function was referenced
prior to its declaration in terms of the order of the lines of code.

Re-ordering the code so that the function declaration occurs prior to
referencing it appears to resolve the error in React Native.

Related: https://git.io/JtXMm

Mobile Error

ios-function-hoisting-error

How has this been tested?

Manually tested modifying the focal point for a Cover block in both the web editor (macOS Safari) and native editor (iOS).

Screenshots

Web
web-focal-point-picker.mov
Native Mobile
native-focal-point-picker.mov

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@dcalhoun
Fix React Native error from function declaration hoisting
5135203 
It is unknown why relying upon function declaration hoisting causes an
error in React Native, but it does occur. Recent changes with WordPress#28676
refactored a class to a functional component, including leveraging
function declaration hoisting. I.e. the function was referenced
prior to its declaration in terms of the order of the lines of code.

Re-ordering the code so that the function declaration occurs prior to
referencing it appears to resolve the error in React Native.

Related: https://git.io/JtXMm
@dcalhoun dcalhoun marked this pull request as ready for review February 15, 2021 22:47
@stokesman
Copy link
Contributor

Wow! So glad you had this idea. Thanks for looking into it! I don't think it would have ever occurred to me.

@dcalhoun dcalhoun mentioned this pull request Feb 15, 2021
Closed
2 tasks
@dcalhoun
Copy link
Member Author

@ItsJonQ will you please review and verify the web editor focal point picker continues to function as expected, given that was the focus of #28676?

@jd-alexander I believe these changes resolve the native mobile editor crash that was introduced in #28676. Will you please review and help verify? Also, I do not have write permissions for gutenberg to request reviewers or attempt to re-run the CI suite. If you are willing to help me manage that, it'd be appreciated. Thank you!

@dcalhoun dcalhoun mentioned this pull request Feb 15, 2021
Merged
6 tasks
@jd-alexander
Copy link
Contributor

@jd-alexander I believe these changes resolve the native mobile editor crash that was introduced in #28676. Will you please review and help verify? Also, I do not have write permissions for gutenberg to request reviewers or attempt to re-run the CI suite. If you are willing to help me manage that, it'd be appreciated. Thank you!

Thanks, @dcalhoun I have added myself and @ItsJonQ as reviewers and I will re-run test(s) in the event any of them fail.

@stokesman
Copy link
Contributor

I've tested this for web. The focal point picker is working as expected. I can't vouch for mobile as I've not yet been able to get the RN/mobile dev env working.

Also, I restarted the tests, probably a second before Joel would've seen it.

jd-alexander
jd-alexander approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@jd-alexander jd-alexander left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for these changes @dcalhoun 😄 I first verified that the function declaration hoisting issue existed on master and then I added your fork's remote, checked out this branch, and verified that the issue has been resolved by utilizing the blocks below since they utilize the withNotices HOC that was refactored in the linked PR.

Blocks tested :

  • Cover's edit focal point functionality
  • Gallery
  • Media-Text
  • Audio block

Thanks again for this fix. LGTM 🚢

@jd-alexander jd-alexander added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Feb 16, 2021
@gziolo
Copy link
Member

gziolo commented Feb 16, 2021

This issue is very strange. Function hoisting is a language feature 🤷

The fix is good but it shouldn’t be necessary. I would expect many places relaying on hoisting in the repository.

@hypest
Copy link
Contributor

hypest commented Feb 16, 2021

Thanks for the fix David and thank you all for being quick at reviewing!

The fix is good but it shouldn’t be necessary.
👋 @gziolo , can I interpret your response there as a LGTM? I can go ahead and merge if so. For context, David and Joel are a few hours away due to timezones and I'd prefer to merge and let the native editor release process resume asap. Thanks!

@gziolo gziolo merged commit 27131b5 into WordPress:master Feb 16, 2021
@github-actions github-actions bot added this to the Gutenberg 10.1 milestone Feb 16, 2021
@dcalhoun dcalhoun changed the title Fix React Native error from function declaration hoisting [RNMobile] Fix React Native error from function declaration hoisting Feb 17, 2021
@dcalhoun dcalhoun deleted the rnmobile/fix-error-from-function-declaration-hoisting branch April 15, 2021 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stokesman stokesman stokesman approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@ItsJonQ ItsJonQ Awaiting requested review from ItsJonQ

+1 more reviewer

@jd-alexander jd-alexander jd-alexander approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Milestone
Gutenberg 10.1
Development

Successfully merging this pull request may close these issues.

Inserting Gallery, Media-Text or Audio block crashes the app
5 participants
@dcalhoun @stokesman @jd-alexander @gziolo @hypest