Small tweaks from ongoing backwards compatibility struggle in plugins

[piemonkey]

Overview

Don't crash if we use a toolbar button for a mark that isn't in the schema and give some more fine-grained output when blackbox tests fail.

connected issues and PRs:

N/A

Setup

N/A

How to test/reproduce

Remove a mark (such as redacted from the schema but leave the corresponding toolbar button in place, you should get a nice console log telling you what's wrong and the button should just be disabled (it used to crash).
Enable one of the failing blackbox tests and see the slightly improved failure message.

Challenges/uncertainties

N/A

Checks PR readiness

• UI: works on smaller screen sizes
• UI: feedback for any loading/error states
• Check cancel/go-back flows
• Check database state correct when deleting/updating (especially regarding relationships)
• changelog
• npm lint
• no new deprecations

[elpoelma]

Suggested change

	if (!args.controller?.schema.marks[args.mark]) {
	if (!args.controller.schema.marks[args.mark]) {

[piemonkey]

This suggestion would just cause it to fail to start as args.controller will be undefined. I'll tweak this to not log on startup. I don't really want to change the way this is used just for this convenience log to work correctly.

[elpoelma]

True, but then we should also adjust the argument types to mark the controller argument as optional.

[piemonkey]

Hmm, this is tricky. In this kind of context, TS assumes that your code is obeying the types, which in a pure TS codebase should (but won't always) be true. At the very least, there should be an error when you misuse something. With a mixture of TS and JS or TS and handlebars, this is no longer true.

I don't think we should change the arguments as while it no longer should crash if controller is not supplied, the class is useless if it isn't. I can't think of a good way to, within this class body, treat Args as partial, but keep the members of Args as required. Just marking get controller() as returning SayController | undefined for example does not seem to work.

[abeforgit]

why would controller be undefined?
that would mean the parent component is passing in an incorrect value, so crashing is totally fine.
Sure, you'd like a build-time warning which we lose because we pass through unchecked templates (we'll get it back when we can enable glint), but that doesn't change how we should design the component imo.
Either you support the flexibility, and type it as optional, and nullcheck yourself, or you don't, and require the calling component to make sure its not passing in undefined.

[elpoelma]

Looks good to me overall! It just seems that the console log shows up a lot, because the controller argument is not passed yet (is still undefined) when constructing the mark-buttons.
Two possible solutions to this:

• Make the controller argument of the buttons non-optional and only instantiate the buttons when the controller is defined
• Move the schema check out of the controller into the mark getter and only check the existance of the mark/node-spec in the schema once the controller is defined.