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

[Gutenberg] Use Hermes bytecode #13297

Closed
wants to merge 11 commits into from
Closed

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Nov 5, 2020

This PR is another attempt after #11436

If it proves more successful I will close the other one.

Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2780

No need to merge this PR per se as it's practically only bumps the gb-mobile hash; the improvements will be included anyway when the gb-mobile side PRs merge.

To test:
No special testing other than all operations should look and feel faster.
One easy to spot improvement (I guess) is to start a new post and notice how much faster the editor comes online.

PR submission checklist:

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 5, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 5, 2020

You can test the changes on this Pull Request by downloading the APK here.

@hypest hypest requested a review from mchowning November 6, 2020 16:03
@hypest hypest marked this pull request as ready for review November 6, 2020 16:03
@hypest hypest added this to the Future milestone Nov 6, 2020
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

This is working great! 🙇 Thank you @hypest !

Spent a fair bit of time testing this with Vanilla, Jalapeno, and Wasabi builds (including a VanillaRelease build). I tried to hit every flow I could think of and everything appears to be working great. 👍

@mchowning
Copy link
Contributor

👋 @hypest ! I've merged the gb and gb-mobile PRs, but I haven't merged this one because you said:

No need to merge this PR per se as it's practically only bumps the gb-mobile hash; the improvements will be included anyway when the gb-mobile side PRs merge.

I'm not sure exactly what you mean when you say "the improvements will be included anyway when the gb-mobile side PRs merge", would you mind expanding on that a bit just so I can understand a bit better?

@hypest
Copy link
Contributor Author

hypest commented Nov 6, 2020

I'm not sure exactly what you mean when you say "the improvements will be included anyway when the gb-mobile side PRs merge", would you mind expanding on that a bit just so I can understand a bit better?

Hey @mchowning, ah, sorry. I mean that the changes/bump will be picked up by the normal editor release process. If there was a need for WPAndroid side changes beyond the hash bump then we'd need to merge them together with the gb-mobile PRs.

At this stage, since the gb-mobile/gutenberg PRs got merged (🎉🙇‍♂️), the upcoming Monday editor cut will pick them up very soon.

@mchowning
Copy link
Contributor

I mean that the changes/bump will be picked up by the normal editor release process... [S]ince the gb-mobile/gutenberg PRs got merged (🎉🙇‍♂️), the upcoming Monday editor cut will pick them up very soon.

Ah, that makes sense. My inclination would be to still merge this PR to ensure (😉) that the rest of the WPAndroid devs are exposed to these changes while our release process is pending next week. In other words, although these changes will get picked up by our release branch, they won't make it to WPAndroid's develop branch until the end of next week, right? I'd hate for the non-gutenberg WPAndroid devs to discover an issue at that point in time. Maybe I'm missing something though, let me know what you think.

@mchowning
Copy link
Contributor

I don't feel that strongly one or the other about merging this, but in case we decide we do want to merge it I went ahead and pushed an update (eef44b5) to this branching bringing in the post-hermes-merge gutenberg-mobile submodule hash.

@hypest
Copy link
Contributor Author

hypest commented Nov 9, 2020

v1.41.0 of the gb-mobile editor has been cut and the WPAndroid PR for that is including the Hermes bytecode work so, it's safe to close this at this stage. Thanks for the review and update Matt!

@hypest hypest closed this Nov 9, 2020
@mchowning mchowning deleted the try/use-hermes-bytecode-nov-2020 branch December 17, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants