WIP: Pulls/4/react dependency injection #70

unclecheese · 2017-05-16T09:57:17Z

~~Don't merge. API in active flux and discussion.~~

unclecheese · 2017-05-16T22:16:35Z

Issues, limitations, and open questions

Based on my observations so far...

Only one component gets to influence `render()`

While the props shape can represent the aggregate influence of all customisations of the component, render() doesn't quite work that way. See experiment 2 and experiment 3 for how this becomes a problem. If the password strength checker component wanted to render the alert message as UI, it won't be able to rely on the core TextField UI. That UI may or may not have already been updated by TextFieldCharacterCountCreator.

Order of execution / priority

Everything in the DI layer will suffer from this problem, but it's something to keep in mind. Should we keep this to a simple priority parameter to customise()? That might invite an arms race between module developers, e.g. {priority: 999999} etc..

The need for context

Right now, all customisations are global. Every single FormBuilder rendered text field in the CMS now has a character count. This is not good enough. We need an API that allows the both the consumer of the injected component to specify context, as well as the provider of the injected component. Rough API:

this.context.injector.get('TextField.CampaignAdmin.EditForm.Title');

Injector.customise('TextField.CampaignAdmin.EditForm.Title', NewComponentCreator);

Ideally, we could make this more flexible with wildcarding:

Injector.customise('TextField.CampaignAdmin.*') // All text fields in CampaignAdmin

Injector.customise('TextField.AssetAdmin.SearchForm.*') // All text fields in AssetAdmin search

Injector.customise('TextField.*.EditForm.*') // All text fields in edit forms in any CMS section

Injector.customise('DropdownField.*') // I don't like Chosen.

propTypes collisions

If I want the title of my GalleryItem to be an inline image, I'm hosed right now, as the title propType is declared upstream as string. We need an API for influencing that and resolving conflicts. This becomes a bit more problematic with things like oneOfType() and shape().

We should throw when `register()` is called more than once for the same service

register() is used to declare the service for the first time, and defines the service that will be fed to the front of the middleware chain (customise()). It is conceivable that you would want something so specific that you just want to override the entire service. This would conceivably nuke all the existing middleware definitions, or at least put them at risk of failure. Possible solution:

Injector.register('TextField', TextField); // OK

Injector.register('TextField', AnotherTextField); // Throws

Injector.register('TextField', AnotherTextField, { force: true }); // OK

Components are far too monolithic to make DI worthwhile

Without the opportunity to inject on small components, e.g. <GalleryItemTitle />, we're inviting injection by subclass, which we really want to discourage.

Future story: Break up large components into smaller, more agnostic pieces.

silverstripe-admin must be authoritative

The admin module has last rights to the injector, and gets to freeze it before boot. A simple way to achieve that is to put silverstripe-admin's injector calls in a document.load event. However, we cannot enforce in any realistic way that other modules don't do that and create some unpredictable race condition.

One possible solution, as noted below is to wrap the admin module's injector calls in document.load and setTimeout(... 0,).

flamerohr

The concept is sound, I'll be giving this a test run later and see if I can get this going in parallel to what you've done :)

flamerohr · 2017-05-17T01:21:44Z

client/src/containers/App/App.js

@@ -8,9 +9,10 @@ import SilverStripeComponent from 'lib/SilverStripeComponent';
 class App extends SilverStripeComponent {
  render() {
    // TODO re-add <div className="app"> wrapper when applying to document.body
-    const Child = React.Children.only(this.props.children);
-    return (Child);
+    return <div>{this.props.children}</div>;


I'm a little cautious of this due to the use of flexbox currently for the App component.

But I know you're aware of it, so I'll leave that to after this pull request is ready :D

Yeah, I haven't seen any issues yet, but I have noted it as a temporary hack that needs to be reviewed before merge.

flamerohr · 2017-05-17T05:04:37Z

client/src/boot/registerComponents.js

+  Injector.register('LabelField', LabelField);
+  Injector.register('TreeDropdownField', TreeDropdownField);
+
+  Injector.freeze();


Do we want to freeze registering more?

asset-admin injects a few more fields as well as potentially other modules.
Or perhaps only freeze protect customize

So the idea here is that we would discourage every module but silverstripe-admin from making Injector mutations on document load. Admin is the only module that gets that privilege, ensuring it comes last. The other way we could do it, if we wanted to be a bit more permissive is to do it on document load, wrapped in a setTimeout(.., 0); to force it to a new thread.

Neither of those are ideal. We need some kind of contract between modules to ensure silverstripe-admin comes last, though.

flamerohr · 2017-05-17T05:12:02Z

client/src/lib/dependency-injection/withInjector.js

+        Component,
+        Object.assign({}, this.props, props),
+        this.props.children
+      );


Is React.createElement() preferred over <Component {...this.props} />?

Either is fine. Sometimes I find React.createElement more expressive, but we should be consistent and use JSX.

flamerohr · 2017-05-17T09:31:15Z

@unclecheese I'm curious what went wrong with both experiments?
experiment 3 seems like all sorts of no with the setTimeout() and alert() lol

Context would be a tricky one, even more so if you throw in wildcards. Most forms are named specifically and don't know their own context except that it could be inferred by the field's id property.

PropTypes could be opened up to be slightly more forgiving, e.g. display types like title could be handled by the castStringToElement() API that we have, which casts strings to <Element>{string}</Element> or if it's contained in an object it will be casted as such:
{ react: ReactComponent } as a react component
{ html: HtmlString } as unescaped html
I forget the other allowed types, but this could be used a bit more to our advantage.
I wouldn't recommend some data PropTypes to be changed though, such as count properties.

I agree with the throwing of an already registered service, I don't agree with a force flag to allow ourselves or modules to override it as such and would be more keen to opt for a customise() call to handle overriding it (this is assuming that priority has been solved somehow first)

Totally agree with breaking up these monolithic components to smaller ones, I'm also keen to start moving presentational components to stateless functional components and move away from es6 classes for them.
If possible, when breaking up these components, work on opening up the PropTypes as mentioned above to be more forgiving when display certain types of data.

Another note about priority, what do you thinking about having a "name" param for the customize() API, which can help identify the customisations a bit more succinctly, the idea is from the way SS handle config.yml files.

chillu · 2017-05-17T23:01:42Z

client/src/components/FormBuilder/FormBuilder.js

+   * @return object|null
+   */
+  getComponentForDataType(dataType) {
+    const { injector: { get } } = this.context;


Yay, great way to solve this (withInjector context HoC)

chillu · 2017-05-17T23:06:01Z

client/src/lib/dependency-injection/container.js

+const register = (key, value) => {
+  di.factory(
+    key,
+    () => compose(...(middlewares[key] || []))(value)


Such ES6, so compose! ❤️

:) I got a bit cheeky here.

unclecheese · 2017-05-17T23:08:32Z

I'm curious what went wrong with both experiments?
experiment 3 seems like all sorts of no with the setTimeout() and alert() lol

Yeah, it's not meant to be anything you'd use in the real world. Just a proof of concept. setTimeout() is used to throttle the onChange event (so it doesn't trigger every keystroke), and the use of alert() was alluded to in the issue I described with render(). Only one customisation really gets to influence the UI. My password checker could render that message as UI (e.g. an error message below the field), but I have no guarantee that TextField at that point is the standard core TextField. It could have been (and is, in that experiment) already customised to render new UI. So the only easy way to render that messaging is with an alert(). Again, just proving, in theory, we can do it, at this phase.

I agree with the throwing of an already registered service, I don't agree with a force flag to allow ourselves or modules to override it as such and would be more keen to opt for a customise() call to handle overriding it (this is assuming that priority has been solved somehow first)

I tend to agree. The only case I see for having a force parameter is that it's consistent with the Injector pattern we have in the backend, which is "Whomever gets there last gets to have their implementation." If five modules inject on Mailer, four of those are completely irrelevant to the execution chain. It's not unrealistic to expect that frontend devs might want the same authority.

Another note about priority, what do you thinking about having a "name" param for the customize() API, which can help identify the customisations a bit more succinctly, the idea is from the way SS handle config.yml files.

Love this idea. We could use it on the displayName property to make the React console a lot more informative, as well.

unclecheese · 2017-05-17T23:15:35Z

I've added the note about ensuring silverstripe-admin authority to the OP.

chillu · 2017-05-17T23:21:37Z

client/src/lib/dependency-injection/withInjector.js

+          `);
+          return;
+        }
+        const resolved = dependencies.map(this.context.injector.get);


So the constructed container could declare dependencies as propTypes, and this HoC resolves them automatically. Does that mean most container components and some presentational components would be wrapped this way in their default export? I assume that doesn't interfere with the ability to further wrap these exported components with customise()?

Yes. The idea is that almost everything should be wrapped in withInjector(), particularly containers, as they by definition have dependencies. Components that shouldn't would be simple presentational components that render only HTML. e.g. const Title = (props) => <h2>{props.children}</h2>;

There's nothing to my knowledge that interferes with an injector-consuming component being customised.

chillu · 2017-05-17T23:36:04Z

If I want the title of my GalleryItem to be an inline image, I'm hosed right now, as the title propType is declared upstream as string. We need an API for influencing that and resolving conflicts. This becomes a bit more problematic with things like oneOfType() and shape().

I think part of the answer here is making components more granular (such as a GalleryItemTitle), and allowing to replace them completely (or HoC wrap them). PropTypes are our API, and the typing should be specific enough to ensure the component can operate with the input variations. If it expects title as a string, that's because it's render() function will expect a string. If you want it to be a component, that's changing some fairly fundamental assumptions of the render() function (and potentially other logic in the component). I'm not a fan of Chris' suggestion on putting casting info in the propType shape (if I understand this correctly).

Order of execution / priority

Could we just sketch this out for now, and leave implementation to a follow-up card? I'd be keen to get clarity if we go down priority vs before/after as part of this cart, and making sure that we don't paint ourselves in a corner. If the consensus is before/after, then we need a JS constraint resolver library, which sounds like scope creep for this card.

The need for context

Service names in DI container are usually just strings. In SS4, we are using suffixes as well (<interface-fqcn>.<arbitrary-suffix>). So this would be a consistent approach - just that we need multiple layers of suffixes here. Wildcarding makes the system a bit more complex, but as long as we don't use regexes or something it feels like this is an appropriate level of flexibility. In particular for forms, we need context specific behaviour based on various criteria. My only worry is that it'll become quite opaque which implementation is chosen - can you think about diagnostics and debugging for this please?

FormBuilder and FormBuilderLoader don't know anything about the current section name, they only care about the schema URL. I guess the schema could contain this metadata? Ideally we keep section names consistent with their FQCN reference as well, which would look ugly in service name references. Outside of form builder, would you see e.g. <GalleryTile> knowing about section names? I want to avoid components knowing too much. Outside of FormBuilder, is it important to know in which section context a component is applied?

flamerohr · 2017-05-22T00:48:33Z

I'm not a fan of Chris' suggestion on putting casting info in the propType shape (if I understand this correctly).

Not quite how it works, for reference it's done with fieldHolder's title fields (leftTitle, rightTitle, Description) and allows the PHP to define html (or HOC to define react) for these fields if required rather than string. It does mean the propType would need to open up and allow for it though, yes...
I think it might be worth creating our own "displayText" prop type, so it can be reused, but that's outside the scope of this story :D

flamerohr

Overall, I think the theory of how this will work is sound and seeing it in the small testcases that you have provides some good foundation to our understanding.
Great for a POC.

A few linting issues, with attention to the fact that our current eslint ruleset doesn't lint object spread properly yet :(

flamerohr · 2017-05-21T23:32:22Z

client/src/lib/dependency-injection/container.js

+ */
+const addDisplayName = (name) => (Component) => {
+  const currentName = (Component.displayName || Component.name || 'Component');
+  Component.displayName = `${name}(${currentName})`;


eslint wouldn't allow modifying param properties

if this is necessary, perhaps add the // eslint-disable-next-line rule-here comment above this line

flamerohr · 2017-05-21T23:37:49Z

client/src/lib/dependency-injection/container.js

+    addDisplayName(meta.name),
+    factory
+  );
+  middlewares[key].push({...meta, factory});


currently our eslint rules do not support object spread :( (and it would need space padding { ...meta, factory })

flamerohr · 2017-05-21T23:38:46Z

client/src/lib/dependency-injection/container.js

+ * @param key The name of the dependency to customise
+ * @param factory The function that will compose the dependency. Gets passed the previous state of composition
+ */
+const customise = (meta, key, factory) => {


meta is missing a docblock description

flamerohr · 2017-05-22T00:07:54Z

client/src/lib/dependency-injection/container.js

+ */
+const load = function load() {
+  for(const key in middlewares) {
+    if(middlewares.hasOwnProperty(key)) {


how about using Object.keys(middlewares).forEach(key => {?
I'd normally reserve for...in and for...of for cases where breaking is beneficial.

flamerohr · 2017-05-22T00:36:33Z

client/src/lib/dependency-injection/withInjector.js

+            withInjector() passed an argument for dependencies that is ${typeof dependencies}. 
+            Must be an array of named dependencies.
+          `);
+          return;


this return is unreachable.
it's also an inconsistent return value it should be return null; if it were still around ;)

flamerohr · 2017-05-22T00:40:45Z

client/src/lib/dependency-injection/withInjector.js

+    }
+  }
+
+  Component.contextTypes = injectorContext;


I have a feeling you may be overwriting other existing contextTypes for the injected Component, would be safer to have Component.contextTypes = Object.assign({}, Component.contextTypes, injectorContext);

but also, this will fail ESLint. As above add the // eslint-disable-next-line rule-here comment

chillu · 2017-05-23T22:41:04Z

client/src/lib/dependency-injection/container.js

+ * e.x.
+ * Injector.update(
+ *   {
+ *     name: 'my-module',


I think this should be called module rather than name, with beforeModule and afterModule counterparts. It's a bit confusing to use name here, I first thought it was the component name.

Per our discussion on Slack, name is now the first parameter of Injector.transform().

chillu · 2017-05-23T22:45:44Z

client/src/lib/dependency-injection/container.js

+      meta[k] !== undefined &&
+      (typeof meta[k] !== 'string' && !Array.isArray(meta[k]))
+    ) {
+      throw new Error(`Injector.update() key ${k} must be a string or array`);


Thanks for doing these descriptive error messages, that really helps!

chillu · 2017-05-23T22:53:29Z

client/src/lib/dependency-injection/container.js

+  const graph = [];
+  let sortedMiddlewares = [];
+
+  middlewareList.forEach(entry => {


So if we apply a HoC in silverstripe-admin/ on a "base component" (can't think of a use case for this at the moment...), does the * allow modules to apply their customisations before?

You can't do anything in front of the base component. It's stored outside of the middleware chain completely. before: '*' will be executed as the first customisation of the base component but not before.

module-a/Component.js module-a/HoC-a.js (applies to Component) module-b/HoC-b.js (applies to Component)

Can I get HoC-b to apply before HoC-a?

Assuming you're using injector for both cases, you'd put {before: 'HOC-a-group-name'} :)

Yeah, that would be

Injector.transform('module-b', update => { update('Component', HOC); }, { before: 'module-a' });

chillu · 2017-05-23T22:54:42Z

client/src/lib/dependency-injection/container.js

+  ContainerAPI.__reset__ = reset;
+}
+
+export default ContainerAPI;


In AirBnB standards, ES6 module exports should match the file name.

chillu · 2017-05-23T22:55:25Z

client/src/lib/dependency-injection/provideInjector.js

+  return InjectorProvider;
+}
+
+export {


Why don't you make this the default export?

Yeah these should be default exports. They're named exports in the main Injector file, and i think this is just legacy.

chillu · 2017-05-23T22:55:55Z

client/src/lib/dependency-injection/withInjector.js

+  return Injectable;
+}
+
+export {


Same here, should this be the default export?

chillu · 2017-05-23T23:01:52Z

client/src/lib/dependency-injection/container.js

+ *     name: 'my-module',
+ *     before: ['another-module']
+ *   },
+ *   wrap => {


That's a pretty weird syntax - particularly if you present it to PHP developers who might misread this as an array (like I just did). It's also mixing terminologies between "update", "callback" and "wrap".

I've looked at this for a few minutes, and have to honestly say that I can't figure out what gets executed when and how without actually stepping through a debugger. Can we make the API surface of this simpler?

We ended up with this syntax:

// module-a.js Injector.transform('module-a.fields', update => { update('TextField', ComponentA); update('DateField', ComponentB); }); Injector.transform('module-a.core', update => { update('Menu', ComponentC); }, {beforeModule: '*'}); // module-b.js Injector.transform('module-b', update => { update('TextField', ComponentD); }, {beforeModule: 'module-a'});

flamerohr · 2017-05-23T10:56:44Z

client/src/lib/tests/Injector-test.js

+  Injector.__reset__();
+});
+
+const expectError = (func) => {


There's a expect(() => <your test>).toThrow(); built in assert from jest :)
I'd prefer using what's out of the box if possible, I had to fix a fair number of these custom expects when I upgraded jest last time.

flamerohr · 2017-05-23T11:01:57Z

client/src/lib/tests/Injector-test.js

+        Injector.register('TestService', TestService);
+        expectError(() => Injector.get('TestService'));
+        Injector.load();
+        expect(Injector.get('TestService')()).toBe('test service');


I'd suggest expect(typeof Injector.get('TestService')).toBe('function'); before this as well

flamerohr · 2017-05-23T23:17:07Z

client/src/lib/dependency-injection/container.js

+ * @param object
+ */
+const validateMeta = (meta) => {
+  if (typeof meta.name === undefined) {


quotes around undefined

flamerohr · 2017-05-23T23:18:24Z

client/src/lib/dependency-injection/container.js

+  }
+  PRIORITIES.forEach(k => {
+    if (
+      meta[k] !== undefined &&


might be a preference of mine... I'd typeof check for explicit undefined values

chillu · 2017-05-25T20:37:58Z

@unclecheese build is failing on NPM_BUILD:

modulesModuleNotFoundError: Module not found: Error: Cannot resolve 'file' or 'directory' ./dependency-injection/Container in /home/travis/build/silverstripe/silverstripe-admin/client/src/lib

flamerohr · 2017-05-25T22:21:01Z

client/src/lib/dependency-injection/inject.js

+  Injected.contextTypes = injectorContext;
+  Injected.displayName = `inject(
+    ${(Component.displayName || Component.name || 'Component')}
+  )`;


This is really nice to have as a default

chillu · 2017-06-26T04:38:25Z

This caused a regression which was fixed with #116

unclecheese force-pushed the pulls/4/react-dependency-injection branch from 9f76ce1 to f38a60c Compare May 16, 2017 10:01

unclecheese mentioned this pull request May 16, 2017

Customise React components through injection silverstripe/silverstripe-framework#6807

Closed

13 tasks

flamerohr reviewed May 17, 2017

View reviewed changes

chillu reviewed May 17, 2017

View reviewed changes

flamerohr suggested changes May 22, 2017

View reviewed changes

unclecheese force-pushed the pulls/4/react-dependency-injection branch from 4cce3bc to 8367d91 Compare May 23, 2017 00:25

This was referenced May 23, 2017

Use new Injector API silverstripe/silverstripe-asset-admin#487

Merged

Docs for React DI silverstripe/silverstripe-framework#6957

Merged

chillu requested changes May 23, 2017

View reviewed changes

flamerohr suggested changes May 24, 2017

View reviewed changes

Aaron Carlino added 12 commits May 25, 2017 12:12

initial build

114240e

provide injector

eef28ab

Context working

dd1e111

it works

60de997

add bottle js, new API

2b5e84a

Refine public API

ba1b034

Proof of concepts working, API finalised

ebfc99e

rebase

38782d1

Several revisions to API, test suite

dc9901b

build

6d817c2

New test, linting

1b2e3e7

New transform() API, before/after rules cleanup

00eaaac

unclecheese force-pushed the pulls/4/react-dependency-injection branch from 6583088 to 00eaaac Compare May 25, 2017 00:18

chillu approved these changes May 25, 2017

View reviewed changes

Allow strings as dependencies in inject(), not just arrays

3155e8c

unclecheese force-pushed the pulls/4/react-dependency-injection branch from 521f73e to 3155e8c Compare May 25, 2017 22:07

flamerohr approved these changes May 25, 2017

View reviewed changes

Linting errors in test

77efc96

unclecheese merged commit df72497 into silverstripe:master May 26, 2017

unclecheese deleted the pulls/4/react-dependency-injection branch May 26, 2017 00:07

WIP: Pulls/4/react dependency injection #70

WIP: Pulls/4/react dependency injection #70

Conversation

unclecheese commented May 16, 2017 • edited Loading

unclecheese commented May 16, 2017 • edited Loading

Issues, limitations, and open questions

Only one component gets to influence render()

Order of execution / priority

The need for context

propTypes collisions

We should throw when register() is called more than once for the same service

Components are far too monolithic to make DI worthwhile

silverstripe-admin must be authoritative

flamerohr left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flamerohr commented May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unclecheese commented May 17, 2017 • edited Loading

unclecheese commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chillu commented May 17, 2017

flamerohr commented May 22, 2017

flamerohr left a 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chillu commented May 25, 2017

Choose a reason for hiding this comment

chillu commented Jun 26, 2017

unclecheese commented May 16, 2017 •

edited

Loading

unclecheese commented May 16, 2017 •

edited

Loading

Only one component gets to influence `render()`

We should throw when `register()` is called more than once for the same service

flamerohr commented May 17, 2017 •

edited

Loading

unclecheese commented May 17, 2017 •

edited

Loading