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

Fix store crash with gutenberg master #1366

Merged
merged 28 commits into from
Sep 27, 2019
Merged

Fix store crash with gutenberg master #1366

merged 28 commits into from
Sep 27, 2019

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Sep 13, 2019

Fixes #1332

This PR brings us back to targeting gutenberg master

Gutenberg PR: WordPress/gutenberg#17228

To test:

  • Check that tests are all green
  • Try to boot the demo app and perform regular actions
  • Try to save and reload a post in particular

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@Tug Tug requested a review from etoledom September 13, 2019 00:44
@Tug Tug self-assigned this Sep 13, 2019
@mchowning
Copy link
Contributor

This is looking good! Did a quick smoke test to see how this was progressing and everything seems to be working.

This branch does show some problems that have been fixed on rnmobile/master since this branch doesn't include those fixes (i.e., separator block crash, ios crash when viewing html). This obviously isn't a problem since rnmobile/master has these fixes, but I wanted to note this in case anyone else ran across them while testing.

@@ -22,17 +22,20 @@ describe( 'RootComponent', () => {

it( 'renders without crashing', () => {
const app = renderer.create( <RootComponent /> );
renderer.act( () => {
jest.runAllTicks();
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting!
Can we add a comment saying why/when is this needed? 🤔

Copy link
Contributor Author

@Tug Tug Sep 20, 2019

Choose a reason for hiding this comment

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

Added one here for the same reason.

It seems that when EDIT_ENTITY_RECORD is dispatched using this generator function it's done so asynchronously, meaning the promise does not resolve immediately even during tests.

Which makes resetEditorBlocks async which means setupEditorState which comes right after is delayed which means isReady will be true after some time which means editorDidMount will also be called after some time.

This is related to the changes made on core data entities so I figured it was safe to update our tests for now. I'll try to find a good explanation for this and add something 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This is very inconvenient to enforce running fake timers but I guess this is what it is. I don't think we use unit tests to test the root component for the web code.

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.

LGTM!

Other than the minor styling issue with the heading level icons (noted in the gutenberg pr) this looks good. Going ahead and approving this since I think that issue is fine to address separately.

@Tug Tug merged commit 36fb499 into develop Sep 27, 2019
@Tug Tug deleted the update/fix-store-crash branch September 27, 2019 15:13
@hypest hypest mentioned this pull request Sep 30, 2019
11 tasks
@JavonDavis JavonDavis mentioned this pull request Oct 22, 2019
1 task
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.

Crash on the latest master on gutenberg repo
4 participants