ENH Convert LiteralField to functional component with legacy support

[emteknetnz]

Issue #2117

I've tested the legacy component works in conjuction with the the webpack-config PR by reaplce the code BackButton.js in asset-admin with this:

import LegacyLiteralField from 'legacy/react-components/LegacyLiteralField';

class BackButton extends LegacyLiteralField {
  getContent() {
    return { __html: 'i am a cool back button' };
  }
}

export { BackButton as Component };

export default BackButton;

And manually update asset-admins node_modules webpack-config externals.js with what's in the webpack-config PR linked on this issue - works as expected

[GuySartorelli]

In terms of approach, this doesn't tell us a lot because we don't have an existing example of a class that extends LiteralField.

For example, using your BackButton code - how would we then convert that class in a way that avoids duplicating all the code that is currently inherited? Or would we just accept that the code is duplicated? How many classes have to extend something before we say "we don't want this duplicated anymore"?

[GuySartorelli]

I'm not quite following what this comment is trying to tell me. I assume you don't mean it thinks the value undefined is literally a component... is this trying to explain why className and extraClass have default values? Maybe "If there are no passed in props, React assumes your component is an uncontrolled component"?

Do you have any docs you can link to (for me, not necessarily in the comment itself) about this and what that actually affects? My understanding was that controlled vs uncontrolled components was more a concept rather than something that React actually cares about in its runtime code.

I wrote all that before I realised you were just keeping the existing comment. Do you know what the comment is trying to convey? If not I propose we remove it, since it doesn't really make any sense to me.

[emteknetnz]

Deleted

[emteknetnz]

For example, using your BackButton code - how would we then convert that class in a way that avoids duplicating all the code that is currently inherited? Or would we just accept that the code is duplicated? How many classes have to extend something before we say "we don't want this duplicated anymore"?

Just accept that it's duplicated, otherwise it's spaghetti code. Keep in mind we virtually never update these fields (I cannot remember the last time we updated one of these), so risk of them getting "out of sync" is very low. Also we're deleting them all in CMS 7 so ongoing maintenance isn't really a concern IMO.

How many classes have to extend something before we say "we don't want this duplicated anymore"?

Not sure that's relevant? We're converting all fields to functional components which don't really so things like DatetimeField (extends DateField, extends TextField, extends InputField) are likely all just going to have all their logic in the single field, maybe some shared helpers or some HOC thing or something, but functional components cannot subclass.

If you were referring to legacy components - they'll just keep extended the other legacy components i.e. LegacyDateField extends LegacyTextField

[GuySartorelli]

Judging by the "we're deleting them all in CMS 7" comment I think I wasn't clear with what I meant - I'm not talking about the legacy class components since their inheritance chain is unbroken. I'm talking about the functional components, since they can't use inheritance after conversion, but do use inheritance before conversion.

Lets use the real example you pointed out - DateField is a class component that extends the TextField class component. When these are both converted into functional components, this inheritance gets broken. This means anything in TextField that is to be reused in DateField can't just be taken for granted anymore.

In our new functional component, how will DateField be able to reuse logic from TextField? Will we duplicate code between these two functional components? Or have a HoC that shares some logic between them? Or.... something else?

Ignore the legacy class components, I think what you're doing in that regard is fine. And ignore examples like LiteralField which don't currently exist in an inheritance chain. I'm only concerned about:

1. What do we do about any logic we're currently inheriting, after we convert to functional components?
2. What do we suggest other developers do if they want to avoid duplicating code when converting their own extends TextField class components into functional components?

functional components [...] are likely all just going to have all their logic in the single field, maybe some shared helpers or some HOC thing or something

Right. It's figuring out what that will be that seems to be important here, in terms of validating the approach. This PR doesn't show that.

Just accept that it's duplicated, otherwise it's spaghetti code.

That's certainly one option, but I'd like to see it in action so we can properly evaluate the approach - and I'd like to evaluate the approach before merging any PRs that convert form field components. Here's a few related inheritance chains that we'll need to consider, and an example of the amount of duplication we might expect:

• NumberField < InputField
• TinyMceHtmlEditorField < TextField < InputField
• DateTimeField < DateField < TextField < InputField
• TimeField < DateField < TextField < InputField

[emteknetnz]

What we do with functional components in terms of preventing code duplication is kind of irrelevant to this PR. We'll work that out on the PRs that update things like DateField.

The purpose of this set of PRs is to demonstrate how we handle the legacy class components and agree on that, not how we handle functional components

[GuySartorelli]

Okay, fair enough. This looks like a good approach for that narrow scope. I'll hold off merging this for now, pending PRs that show how we'll deal with the functional components of pre-existing class component extension chains as I'd like to know we have a good approach for that. Otherwise we risk merging this only to discover there are problems with converting the rest of them and having to revert.

Will merge when the rest of the approach has been demonstrated.