Skip to content

Conversation

@tyxla
Copy link
Member

@tyxla tyxla commented May 31, 2019

WIP. This PR aims to move all of Sync to a separate, independent package.

Note: this is far from ready to test or use right now. Tests and coding standards need some ❤️

Changes proposed in this Pull Request:

  • Move Sync to a separate package.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • TBD

Testing instructions:

  • TBD

Proposed changelog entry for your changes:

  • TBD

@tyxla tyxla requested a review from a team May 31, 2019 13:45
@tyxla tyxla requested a review from a team as a code owner May 31, 2019 13:45
@tyxla tyxla self-assigned this May 31, 2019
@jetpackbot
Copy link
Collaborator

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against 228b137

"require": {},
"autoload": {
"psr-4": {
"Automattic\\Jetpack\\Sync\\": "src/"
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem like there is a src folder in this package. How does this work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I had that locally, but didn't end up committing it 😉 It's in the branch now.

* This is used to provide compression and serialization to messages
**/
interface iJetpack_Sync_Codec {
interface Codec {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should suffix interfaces with Interface. So that it is more clear that we are implementing one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that, I'll keep it in mind for the next iteration 👍

@roccotripaldi roccotripaldi changed the base branch from master to feature/jetpack-packages May 31, 2019 19:44
@dereksmart dereksmart force-pushed the feature/jetpack-packages branch from 7a4d3f3 to 7df90c1 Compare May 31, 2019 20:28

foreach ( Jetpack_Sync_Modules::get_modules() as $module ) {
foreach ( Modules::get_modules() as $module ) {
Jetpack_Options::delete_raw_option( "{$prefix}_{$module->name()}_sent" );
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have a Jetpack_Options package? @zinigor and @gititon are also using these in #12399 and #12516

how do you all imagine sharing these?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think we should take care of those first, and then start on the sync one.

@tyxla
Copy link
Member Author

tyxla commented Jun 3, 2019

Folks, I'm going to close this PR as it has grown too huge.

Going to take it in smaller steps, but also will make sure that the important packages that sync needs (options, constants, etc.) have landed as separate packages already.

In the next iteration also consider the suggestions everyone here has made.

Thanks!

@tyxla tyxla closed this Jun 3, 2019
@tyxla tyxla deleted the try/sync-package branch June 3, 2019 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants