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.

Sign up for GitHub

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

Try bundling sync package #54738

Merged
merged 5 commits into from
Sep 25, 2023
Merged

Try bundling sync package #54738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6ff9af
Add sync package to bundled packages list
mikachan Sep 22, 2023
9e3bedf
Wrap outputs in GB plugin check
mikachan Sep 22, 2023
4a3e272
Add sync to bundled packages in package config
tellthemachines Sep 25, 2023
1197e1a
Move plugin checks to core data
tellthemachines Sep 25, 2023
128d11f
Move IS_GUTENBERG_PLUGIN inside __experimentalEnableSync checks
mikachan Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions packages/core-data/src/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,12 +385,14 @@ export const editEntityRecord =
}, {} ),
};
if ( window.__experimentalEnableSync && entityConfig.syncConfig ) {
const objectId = entityConfig.getSyncObjectId( recordId );
getSyncProvider().update(
entityConfig.syncObjectType + '--edit',
objectId,
edit.edits
);
if ( process.env.IS_GUTENBERG_PLUGIN ) {
const objectId = entityConfig.getSyncObjectId( recordId );
getSyncProvider().update(
entityConfig.syncObjectType + '--edit',
objectId,
edit.edits
);
}
} else {
if ( ! options.undoIgnore ) {
select.getUndoManager().addRecord(
Expand Down
10 changes: 8 additions & 2 deletions packages/core-data/src/entities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,11 @@ export const getOrLoadEntitiesConfig =
let configs = select.getEntitiesConfig( kind );
if ( configs && configs.length !== 0 ) {
if ( window.__experimentalEnableSync ) {
registerSyncConfigs( configs );
if ( process.env.IS_GUTENBERG_PLUGIN ) {
registerSyncConfigs( configs );
}
}

return configs;
}

Expand All @@ -415,8 +418,11 @@ export const getOrLoadEntitiesConfig =

configs = await loader.loadEntities();
if ( window.__experimentalEnableSync ) {
registerSyncConfigs( configs );
if ( process.env.IS_GUTENBERG_PLUGIN ) {
registerSyncConfigs( configs );
}
}

dispatch( addEntities( configs ) );

return configs;
Expand Down
66 changes: 34 additions & 32 deletions packages/core-data/src/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,39 +79,41 @@ export const getEntityRecord =
entityConfig.syncConfig &&
! query
) {
const objectId = entityConfig.getSyncObjectId( key );

// Loads the persisted document.
await getSyncProvider().bootstrap(
entityConfig.syncObjectType,
objectId,
( record ) => {
dispatch.receiveEntityRecords(
kind,
name,
record,
query
);
}
);
if ( process.env.IS_GUTENBERG_PLUGIN ) {
const objectId = entityConfig.getSyncObjectId( key );

// Loads the persisted document.
await getSyncProvider().bootstrap(
entityConfig.syncObjectType,
objectId,
( record ) => {
dispatch.receiveEntityRecords(
kind,
name,
record,
query
);
}
);

// Boostraps the edited document as well (and load from peers).
await getSyncProvider().bootstrap(
entityConfig.syncObjectType + '--edit',
objectId,
( record ) => {
dispatch( {
type: 'EDIT_ENTITY_RECORD',
kind,
name,
recordId: key,
edits: record,
meta: {
undo: undefined,
},
} );
}
);
// Boostraps the edited document as well (and load from peers).
await getSyncProvider().bootstrap(
entityConfig.syncObjectType + '--edit',
objectId,
( record ) => {
dispatch( {
type: 'EDIT_ENTITY_RECORD',
kind,
name,
recordId: key,
edits: record,
meta: {
undo: undefined,
},
} );
}
);
}
} else {
if ( query !== undefined && query._fields ) {
// If requesting specific fields, items and query association to said
Expand Down
1 change: 1 addition & 0 deletions packages/dependency-extraction-webpack-plugin/lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const BUNDLED_PACKAGES = [
'@wordpress/icons',
'@wordpress/interface',
'@wordpress/undo-manager',
'@wordpress/sync',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😅

Copy link
Member

Choose a reason for hiding this comment

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

If that helps move things forward, it's also possible to land these changes only in wp/6.4 branch.

];

/**
Expand Down
1 change: 1 addition & 0 deletions tools/webpack/packages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const BUNDLED_PACKAGES = [
'@wordpress/icons',
'@wordpress/interface',
'@wordpress/undo-manager',
'@wordpress/sync',
];

// PHP files in packages that have to be copied during build.
Expand Down