Skip to content

Fix race condition that's causing preview/console and code to be out of sync (#990) #1031

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

Merged
merged 4 commits into from
May 2, 2019

Conversation

ZiyaoWei
Copy link
Contributor

@ZiyaoWei ZiyaoWei commented Apr 13, 2019

This change attempts to fix #990. The cause seems to be a race condition - similar to #790 - where the state stored in redux is slightly behind the editor. This PR pulls the current file from the editor and use its content instead of the state in redux to compute the preview and console output.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
  • is descriptively named and links to an issue number, i.e. Fixes #123

@ZiyaoWei
Copy link
Contributor Author

Not sure if this is an okay fix, and any feedback are welcomed!

@catarak
Copy link
Member

catarak commented Apr 17, 2019

thanks for this! i think this is the right approach.

@ZiyaoWei ZiyaoWei force-pushed the wzy/990/fixPreview branch from 4da77a2 to 2fc613b Compare April 25, 2019 01:45
@catarak
Copy link
Member

catarak commented May 1, 2019

i'm setting an error in the console:

webpack-internal:///99:20 Warning: Failed prop type: The prop `cmController` is marked as required in `PreviewFrame`, but its value is `undefined`.
    in PreviewFrame (created by IDEView)
    in IDEView (created by Connect(IDEView))
    in Connect(IDEView) (created by withRouter(Connect(IDEView)))
    in withRouter(Connect(IDEView)) (created by RouterContext)
    in div (created by App)
    in App (created by Connect(App))
    in Connect(App) (created by RouterContext)
    in RouterContext (created by Router)
    in Router (created by App)
    in Provider (created by App)
    in App (created by HotExportedApp)
    in AppContainer (created by HotExportedApp)
    in HotExportedApp

I think this is happening because there's a moment when the CodeMirror editor hasn't been instantiated yet. What I did in Nav.jsx (which also has a cmController prop) was make a default value for cmController that's an empty object.

@ZiyaoWei ZiyaoWei force-pushed the wzy/990/fixPreview branch from 87d15ec to c3aff9f Compare May 2, 2019 04:34
@ZiyaoWei
Copy link
Contributor Author

ZiyaoWei commented May 2, 2019

Good catch, thanks! Hopefully this latest commit fixed that.

@catarak
Copy link
Member

catarak commented May 2, 2019

cool, this is working great for me. merging!

@catarak catarak merged commit 10403a7 into processing:master May 2, 2019
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.

Inconsistency between the results and the code
2 participants