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.

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

Register Internal and Custom backend API's #1011

Merged
merged 3 commits into from
Jan 25, 2018
Merged

Conversation

talves
Copy link
Collaborator

@talves talves commented Jan 15, 2018

- Summary

A solution to register a custom backend API while also registering the internal API's so there is no overwriting of names.

The backends internally target github and the git-gateway without any changes to the CMS itself. This solution is a non breaking solution for the current CMS without having to change the internal backend's.

- Test plan

Make sure the CMS works as is using all current internal backends.
Test a custom created backend by registering the new backend CMS.registerBackends("my-custom-backend", MyCustomBackendClass)
Created an example backend library here (might look familiar to some of you)

- Description for the changelog

Register Internal and Custom backend API's

- A picture of a cute animal (not mandatory but encouraged)
🐶

@verythorough
Copy link
Contributor

verythorough commented Jan 15, 2018

Deploy preview for netlify-cms-www ready!

Built with commit dd3a364

https://deploy-preview-1011--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Jan 15, 2018

Deploy preview for cms-demo ready!

Built with commit dd3a364

https://deploy-preview-1011--cms-demo.netlify.com

/**
* Backend API
*/
export function registerBackends(name, BackendClass) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be singular, since you are only registering one backend at a time (with each function call). For example, we have registerWidget.

Copy link
Collaborator Author

@talves talves Jan 15, 2018

Choose a reason for hiding this comment

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

I thought so too 👍 of course, after I changed it 😄 [Changed Back!]

console.error(`Backend [${ name }] already registered. Please choose a different name.`);
} else {
registry.backends[name] = {
init: config => new BackendClass(config),
Copy link
Contributor

@tech4him1 tech4him1 Jan 15, 2018

Choose a reason for hiding this comment

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

I still wonder why we shouldn't just do

registry.backends[name] = BackendClass;

and then in the backend code

const UserBackend = getBackend(name);
return new Backend(new UserBackend(config), name, authStore);

Can you explain? Just cleaner?

Copy link
Collaborator Author

@talves talves Jan 15, 2018

Choose a reason for hiding this comment

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

coder preference, I like the object reference is all. Kept it the way it was.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for taking initiative on this @talves, I love how simple of a first step this is!

Regarding this piece of code, we want project style to trump an individual developer's preference - no reason to make this function different than the others. Let's update this registry function to simply assign the registered entity as Caleb suggested.

After that, should be good to merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't agree.

Copy link
Contributor

@tech4him1 tech4him1 Jan 24, 2018

Choose a reason for hiding this comment

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

I'm fine either way, it's pretty much just bikeshedding IMO. Do we have a reason that it should be one way or another?

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be different if we weren't committing to this decision as a part of our public API, which we can't break until 2.0 once merged. We should be in agreement before proceeding - let's feature flag it for now and get some mileage on it before making it public.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like Tony said, this specific discussion is just about code style -- it doesn't impact the public API at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

On not breaking the public API, though, I'm fine with putting it in a beta/flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack, this does not impact the public API, I stand corrected 🤦‍♂️

*/
registerBackends('git-gateway', GitGatewayBackend);
registerBackends('github', GitHubBackend);
registerBackends('test-repo', TestRepoBackend);
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@talves
Copy link
Collaborator Author

talves commented Jan 15, 2018

Created an example backend library here (might look familiar to some of you)

@tech4him1
Copy link
Contributor

tech4him1 commented Jan 20, 2018

Discussion on handling dependencies in Gitter (January 15, 2018 4:34 PM):

@tech4him1
"regular" backends like GitLab will need that as well

import { filterPromises, resolvePromiseProperties } from "Lib/promiseHelper";
import AssetProxy from "ValueObjects/AssetProxy";
import { status } from "Constants/publishModes";
import { APIError, EditorialWorkflowError } from "ValueObjects/errors";

but
that can be changed/added later

@biilmann
hmm, the promiseHelper looks like something that we might just want to extract in a more general helper library if we need it?
and then it might make sense to do dependency injection for the value objects and constants instead of importing them

@tech4him1
yeah, ValueObjects and Constants are pretty much required for Editorial Workflow, so I think that would make a lot of sense

@talves
I am thinking I might turn the FS backend into a stand alone library, and then it can be shipped around too
Would also take the FS out of the CMS and anyone could change it the way they want

@tech4him1
the way that you set it up with an backend.init(config) should make dep injection easy, too

@talves
yeah, good point
AssetProxy is the only one that would be difficult

@tech4him1
which all we're using if for, AFAIK, is instanceof checking

@talves
how would we do it simple and clean?
through the init?

@tech4him1
yeah, I'd say either inject it into the scope like Matt said, or pass it into the class constructor

@talves
yeah, the class constructor could just ignore it if it was not needed

console.error(`Backend [${ name }] already registered. Please choose a different name.`);
} else {
registry.backends[name] = {
init: config => new BackendClass(config),
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for taking initiative on this @talves, I love how simple of a first step this is!

Regarding this piece of code, we want project style to trump an individual developer's preference - no reason to make this function different than the others. Let's update this registry function to simply assign the registered entity as Caleb suggested.

After that, should be good to merge.

Copy link
Contributor

@tech4him1 tech4him1 left a comment

Choose a reason for hiding this comment

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

LGTM

@erquhart erquhart merged commit 7053ccd into master Jan 25, 2018
@erquhart erquhart deleted the registry-backend branch January 25, 2018 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants