jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

Extensions: register plugins from a single index file

Open retrofox opened this issue 4 years ago • 9 comments

Registering the plugins from a single order will help to set the importing order, and thus and depending on the plugin, it will be reflected in the UI. For instance, we register the Publicize plugin in the first place of the list because we want to render it at the top of the sidebar.

Fixes #21036 Related with https://github.com/Automattic/jetpack/issues/16816

Changes proposed in this Pull Request:

  • Extensions: register block-editor plugins from a single index file

Jetpack product discussion

Does this pull request change what data or activity we track or use?

No

Testing instructions:

  • Apply the changes
  • Confirm the sidebar looks as expected.
  • No visual changes should happen compared with prod.

retrofox avatar Sep 13 '21 12:09 retrofox

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • :white_check_mark: Include a description of your PR changes.
  • :white_check_mark: All commits were linted before commit.
  • :red_circle: Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • :white_check_mark: Add testing instructions.
  • :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  • :white_check_mark: Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation :robot:


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Then, add the "[Status] Needs Team review" label and ask someone from your team review the code. Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: October 5, 2021.
  • Scheduled code freeze: September 28, 2021.

github-actions[bot] avatar Sep 13 '21 12:09 github-actions[bot]

Does https://github.com/Automattic/jetpack/blob/master/projects/plugins/jetpack/extensions/README.md#upgrades-for-jetpack-sidebar-extensions need updating? I'm not very clear on the difference between "blocks" and "plugins".

anomiex avatar Sep 13 '21 15:09 anomiex

Does https://github.com/Automattic/jetpack/blob/master/projects/plugins/jetpack/extensions/README.md#upgrades-for-jetpack-sidebar-extensions need updating?

I've read that section it seems to be ok from my PoV. I can add more info about these changes.

I'm not very clear on the difference between "blocks" and "plugins".

Not sure whether it should be described here either since these concepts are part of the core API. We could add more external links, though.

retrofox avatar Sep 13 '21 15:09 retrofox

This is still a lot of boilerplate.

If I understand it correctly, we already have an ordered list of things to register in projects/plugins/jetpack/extensions/index.json. The problem is that since #20944 it's now putting all the ones in plugins/ first and then all the ones in blocks/, or vice versa, instead of preserving the order from that file. Am I missing something?

If I'm not, why don't we just change the code from #20944 to preserve the order from the json file?

function presetProductionExtensions( type, inputDir, presetBlocks ) {
    return presetBlocks
        .flatMap( block =>
            blockEditorDirectories.map( dir => path.join( inputDir, dir, block, `${ type }.js` ) )
        )
        .filter( fs.existsSync );
}

anomiex avatar Sep 13 '21 19:09 anomiex

If I understand it correctly, we already have an ordered list of things to register in projects/plugins/jetpack/extensions/index.json.

Correct.

The problem is that since #20944 it's now putting all the ones in plugins/ first and then all the ones in blocks/, or vice versa, instead of preserving the order from that file.

Yes, that makes sense.

If I'm not, why don't we just change the code from #20944 to preserve the order from the json file?

Could it get a little bit confusing? How do we deal with beta, experimental, no-post-editor extensions? I prefer registering the plugin explicitly in a plugins list instead, honestly.

retrofox avatar Sep 13 '21 19:09 retrofox

Could it get a little bit confusing? How do we deal with beta, experimental, no-post-editor extensions?

How would you deal with those in the context of this PR?

For the record: With the index.json, the "experimental" list currently gets appended to the "production" list, and the "beta" list gets appended to the "production+experimental" list. The "no-post-editor" list is a standalone list.

anomiex avatar Sep 13 '21 20:09 anomiex

Could it get a little bit confusing? How do we deal with beta, experimental, no-post-editor extensions?

How would you deal with those in the context of this PR?

Basically it's what the PR does:

import registerJetpackPlugin from '../shared/register-jetpack-plugin';

// Import plugins.
import { name as publicizePluginName, settings as publicizePluginSettings } from './publicize';
import { name as sharingPluginName, settings as sharingPluginSettings } from './sharing';

// list. Order matters.
const jetpackPlugins = [
	[ publicizePluginName, publicizePluginSettings ],
	[ sharingPluginName, sharingPluginSettings ],
];

// Register.
jetpackPlugins.forEach( ( [ name, settings ] ) => registerJetpackPlugin( name, settings ) );

Just FYI, the only change that we introduce here is, instead of registering the plugin in a file for each plugin, it imports and registers the plugins in a single file. It isn't mandatory, and the current implementation will keep working as usual. It's needed when we need to set, explicitly, the registering order.

retrofox avatar Sep 14 '21 11:09 retrofox

I don't see anywhere in there where you're handling beta, experimental, or no-post-editor extensions. You're just registering everything, assuming it's not beta or experimental and always no-post-editor.

Speaking of which, this PR seems to be adding the two plugins to the no-post-editor bundle where they weren't there before. Is that intended?

anomiex avatar Sep 14 '21 13:09 anomiex

Speaking of which, this PR seems to be adding the two plugins to the no-post-editor bundle where they weren't there before. Is that intended?

No, it shouldn't. Let me take a look again, but it's definitely something to change. Maybe you're right. We probably want to take your suggestion first.

retrofox avatar Sep 14 '21 17:09 retrofox

Closing this for now because of the lack of activity on this. We can always reopen in the future if needed.

jeherve avatar Mar 29 '23 07:03 jeherve