Try bundling sync package#54738
Merged
Merged
Conversation
mikachan
added
[Type] Enhancement
|
Size Change: +470 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1197e1a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6294737410
|
youknowriad
reviewed
Sep 22, 2023
youknowriad
reviewed
Sep 22, 2023
| '@wordpress/icons', | ||
| '@wordpress/interface', | ||
| '@wordpress/undo-manager', | ||
| '@wordpress/sync', |
Contributor
There was a problem hiding this comment.
bundling sync package is only necessary if the "tree shaking didn't exclude the package somehow" on Core (no white page).
If we're still experiencing issues, we can consider bundling. There are two files to update, this one and tools/webpack/packages.js
Member
There was a problem hiding this comment.
Tree shaking probably doesn't work in the development mode, so this might behave strangely for the unminified version of the wp-core-data script when not using bundling. I haven't tested, but I guess that we would see wp.sync global replacing the import and the referenced code would never run because it's behind the feature flag. So the only issue would be really the wp-sync dependency listed in the asset file. Well, we could always register that entry point in the dev mode and it shouldn't be a big deal 🤷🏻
It definitely needs some testing as this is quite advanced usage. If @youknowriad has some recommendation, I would follow his guidance, as he knows best what is the desired configuration in the Gutenberg plugin.
Contributor
There was a problem hiding this comment.
My hesitation is whether the dependency extraction happens before or after tree shaking. If it happens before, we're screwed and we need to "bundle" (but it's fine because the bundled code is going to be tree shaken in core anyway), but if the webpack dependency extraction happens after tree shaking, then the dependency to wp-sync won't be added in core, so everything would work.
Contributor
There was a problem hiding this comment.
Is there any downside at all to trying to bundle now? My concern is that Beta 1 is tomorrow and the package update in core is pending. Would be good to get this sorted out now 😅
Contributor
There was a problem hiding this comment.
@tellthemachines No big downsides no, the only one I see is that some people could have started using the wp-sync global in WordPress and bundling would break that.
Contributor
There was a problem hiding this comment.
Good point, but given the sync package is still highly experimental that doesn't seem a huge risk. I'd say it's worth it to mitigate core update issues for this release 😅
Member
There was a problem hiding this comment.
If that helps move things forward, it's also possible to land these changes only in wp/6.4 branch.
tellthemachines
left a comment
tellthemachines
left a comment
Contributor
There was a problem hiding this comment.
Thanks for getting this started @mikachan!
I updated the PR based on the feedback so far. It's hard to tell if this change is going to work to exclude the dependency in core or not and there's no way of testing that I know of without merging this and publishing the packages 😄
If @youknowriad and @gziolo have no objections, I'm in favour of giving it a try.
| '@wordpress/icons', | ||
| '@wordpress/interface', | ||
| '@wordpress/undo-manager', | ||
| '@wordpress/sync', |
Contributor
There was a problem hiding this comment.
Is there any downside at all to trying to bundle now? My concern is that Beta 1 is tomorrow and the package update in core is pending. Would be good to get this sorted out now 😅
youknowriad
reviewed
Sep 25, 2023
| if ( window.__experimentalEnableSync ) { | ||
| registerSyncConfigs( configs ); | ||
| if ( process.env.IS_GUTENBERG_PLUGIN ) { | ||
| if ( window.__experimentalEnableSync ) { |
Contributor
There was a problem hiding this comment.
Could these two checks be combined? (same for the other one)
Contributor
There was a problem hiding this comment.
They can as long as we add // eslint-disable-next-line @wordpress/is-gutenberg-plugin above (the lint rule doesn't seem to recognise if statements with multiple conditions)
Contributor
There was a problem hiding this comment.
ohhh, I think that rule is actually important to follow because that's what allows the tree shaking, so I actually we shouldn't do that (even in the case where it's done now in the PR)
Contributor
There was a problem hiding this comment.
Oooh I see. For the places where I disabled the rule, I guess we could wrap the whole block in the condition, but then we'd have to duplicate some of the logic to make sure the else part underneath still runs if we're in the plugin but have the experiment disabled. @mikachan are you able to take over? It's dinner time for me 😅
Contributor
There was a problem hiding this comment.
... or would it work if we added the IS_GUTENBERG_PLUGIN check inside the window.__experimentalEnableSync one?
Member
Author
There was a problem hiding this comment.
... or would it work if we added the IS_GUTENBERG_PLUGIN check inside the window.__experimentalEnableSync one?
I've just tried this and the linter seemed happy 👀
Member
Author
There was a problem hiding this comment.
@youknowriad, do you think the latest change that moves the IS_GUTENBERG_PLUGIN checks is good to merge?
youknowriad
approved these changes
Sep 25, 2023
gziolo
approved these changes
Sep 25, 2023
mikachan
added a commit
that referenced
this pull request
Sep 25, 2023
* Add sync package to bundled packages list * Wrap outputs in GB plugin check * Add sync to bundled packages in package config * Move plugin checks to core data * Move IS_GUTENBERG_PLUGIN inside __experimentalEnableSync checks --------- Co-authored-by: tellthemachines <isabel@tellthemachines.com>
Member
Author
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: fb1ebb1 |
mikachan
removed
the
Backport to Gutenberg RC
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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?
This adds the sync package to the list of bundled packages, and wraps related code in a
if ( process.env.IS_GUTENBERG_PLUGIN )check.Why?
To prevent Webpack from seeing it as a dependency when syncing with Core. Please see more details here: WordPress/wordpress-develop#5262 (comment)
How?
Adds the sync package to the list of bundled packages, and wraps related code in a
if ( process.env.IS_GUTENBERG_PLUGIN )check.I have no idea if I've approached this correctly, but I wanted to kick-off the work as it's necessary in order to complete the sync work between Gutenberg and Core ahead of the WordPress 6.4 Beta 1 deadline. Please feel free to edit as needed.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast