-
Notifications
You must be signed in to change notification settings - Fork 98
Fix FM module loader in Gutenberg context #815
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
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
f608542
Fix FM module loader in Gutenberg context
kjbenk 35cc471
Properly check for Gutenberg editor
kjbenk c25c3ca
Refactor logic into a wrapped callback function
kjbenk 8268558
Remove update to 1.3.0
kjbenk 08926e6
Use a polling function for firing callbacks in the block editor.
joemcgill 1f4697f
Decrement counter for polling attempts
joemcgill 167bf4d
Fix whitespace change
joemcgill 6f00450
Rely on state changes and remove polling function
joemcgill 621cfa3
Consolodate nested if statements
joemcgill 0bf49fb
Merge branch 'main' into fix/fm-loader-gutenberg
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we feel this is always going to be sufficient? It's only as little as a half a second -- but, if loading the meta boxes takes longer than that, I expect it would be blocking the event loop, so that's probably moot?
Using timeouts or intervals still feels like a flawed pattern to me. For what it's worth,
setIntervaldoesn't appear once in the GB codebase. And as far as I could tell with a quick pass,setTimeoutwas only used in cases like animations, flashes, etc.Further, is an interval even necessary? Once subscribed, won't this repeatedly get executed during ~any activity in the editor? And to that point, should this get properly unsubscribed once successfully executed? Does GB subscribe work like redux subscribe in that you get an unsubscriber and can unsubscribe? I actually don't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it looks like subscribing a listener returns a function to unsubscribe it, as with redux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW @joemcgill here is how I implemented the unsubscribe logic - c25c3ca#diff-92affb4a9da5be89b7b8cf47a3822c1eba297ee97c9f7487feaea5f214726c45R33