Register Internal and Custom backend API's #1011

talves · 2018-01-15T20:46:05Z

- 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 · 2018-01-15T20:47:57Z

Deploy preview for netlify-cms-www ready!

Built with commit dd3a364

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

verythorough · 2018-01-15T20:48:30Z

Deploy preview for cms-demo ready!

Built with commit dd3a364

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

tech4him1 · 2018-01-15T23:11:44Z

src/lib/registry.js

+/**
+ * Backend API
+ */
+export function registerBackends(name, BackendClass) {


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.

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

tech4him1 · 2018-01-15T23:15:58Z

src/lib/registry.js

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


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?

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

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.

I don't agree.

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?

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.

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

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

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

tech4him1 · 2018-01-15T23:17:14Z

src/backends/backend.js

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


talves · 2018-01-15T23:25:36Z

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

tech4him1 · 2018-01-20T17:21:04Z

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

erquhart · 2018-01-20T17:34:54Z

src/lib/registry.js

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


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.

tech4him1

LGTM

talves added 2 commits January 15, 2018 09:26

allow backend register

3675933

handle multiple backends, including internal

32364a6

tech4him1 reviewed Jan 15, 2018

View reviewed changes

back to singular 😄

dd3a364

talves requested review from Benaiah, erquhart and tech4him1 January 16, 2018 22:57

talves mentioned this pull request Jan 20, 2018

File-system backend fixes and support JWT #1029

Closed

erquhart suggested changes Jan 20, 2018

View reviewed changes

tech4him1 approved these changes Jan 24, 2018

View reviewed changes

erquhart approved these changes Jan 25, 2018

View reviewed changes

erquhart merged commit 7053ccd into master Jan 25, 2018

erquhart deleted the registry-backend branch January 25, 2018 16:55

erquhart mentioned this pull request Jan 25, 2018

File System API (Final On-Hold) See Comments #786

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register Internal and Custom backend API's #1011

Register Internal and Custom backend API's #1011

talves commented Jan 15, 2018 •

edited

Loading

verythorough commented Jan 15, 2018 •

edited

Loading

verythorough commented Jan 15, 2018 •

edited

Loading

tech4him1 Jan 15, 2018

talves Jan 15, 2018 •

edited

Loading

tech4him1 Jan 15, 2018 •

edited

Loading

talves Jan 15, 2018 •

edited

Loading

erquhart Jan 20, 2018

talves Jan 24, 2018

tech4him1 Jan 24, 2018 •

edited

Loading

erquhart Jan 25, 2018

tech4him1 Jan 25, 2018

tech4him1 Jan 25, 2018

erquhart Jan 25, 2018

tech4him1 Jan 15, 2018

talves commented Jan 15, 2018

tech4him1 commented Jan 20, 2018 •

edited

Loading

erquhart Jan 20, 2018

tech4him1 left a comment

Register Internal and Custom backend API's #1011

Register Internal and Custom backend API's #1011

Conversation

talves commented Jan 15, 2018 • edited Loading

verythorough commented Jan 15, 2018 • edited Loading

verythorough commented Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

talves Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

tech4him1 Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

talves Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4him1 Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talves commented Jan 15, 2018

tech4him1 commented Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

tech4him1 left a comment

Choose a reason for hiding this comment

talves commented Jan 15, 2018 •

edited

Loading

verythorough commented Jan 15, 2018 •

edited

Loading

verythorough commented Jan 15, 2018 •

edited

Loading

talves Jan 15, 2018 •

edited

Loading

tech4him1 Jan 15, 2018 •

edited

Loading

talves Jan 15, 2018 •

edited

Loading

tech4him1 Jan 24, 2018 •

edited

Loading

tech4him1 commented Jan 20, 2018 •

edited

Loading