Polyfills: exclude web.immediate #49234
Merged
Conversation
This file contains 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
|
Size Change: +620 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 70c7d74. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4480504475
|
gziolo
reviewed
Mar 23, 2023
gziolo
reviewed
Mar 23, 2023
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
gziolo
approved these changes
Mar 23, 2023
|
Thank you for the review, @gziolo! 👍 |
|
@sgomes, @gziolo, should we override For the context: The behavior to use |
|
@sgomes, as you note in the PR description, I'm not sure why the plugin is not doing that already, since it overrides 99% of scripts used by the editor(s). See https://github.com/WordPress/gutenberg/blob/trunk/lib/client-assets.php#L185-L239. |
|
@Mamaduka Oh, I see, thank you! 👍 So if I understand you correctly, you're suggesting that we override |
Correct! We can also ship similar fixes faster to folks who use the plugin. |
|
Good idea 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Remove
setImmediate/clearImmediatefrom the list of features that we polyfill.Why?
While investigating some performance issues in Safari, I noticed that
setImmediate/clearImmediateare being polyfilled bycore-js. These are IE-only features which we don't use, and therefore don't need polyfills for. That alone is probably reason enough to exclude them.But beyond that, it's worth pointing out that
reactdoes look for them in some circumstances, when deciding how to schedule work. It prefers the nativesetImmediateimplementation when available vs other functionality (such as message channels), due to some inherent characteristics of how it's implemented. So when we polyfillsetImmediate, we trick React into opting for this approach, even though the inherent characteristics it prefers are not there (since it's just a polyfill that's implemented on top of other functionality).It's therefore best to simply have
reactpick one of the other approaches, instead of havingcore-jsspend the effort to poorly reimplementreact's preferred approach.How?
It excludes the aforementioned features by changing the
babel-preset-defaultconfiguration.Testing Instructions
It's not entirely clear to me how to force the testing environment to load a newly-generated set of polyfills, given that it seems to be controlled via
load-scripts.php(which, in my testing, didn't pick up the modified polyfills).(I had to resort to local overrides in my browser to manually modify the concatenated script)
Assuming you can get the new polyfills to load, however, it's simple: just load the editor and smoke-test any functionality, as any problems would make themselves evident immediately. If possible, test using Safari, which does follow the
reactscheduling codepath in question.Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A