Feature/derived problems

[XanderVertegaal]

Lets the user copy existing problems. Derived problems have a reference to their 'base' problem.

[jgonggrijp]

I have excluded the changes that are also in #57 from my review.

I have some nitpicks, but overall the changes look perfectly reasonable to me.

Note: when I read back my own comments in the Conversation tab, the first two are placed under the wrong snippet of code. Maybe that is because I skipped one commit in my review. They appear in the right place in the "Files changed" tab.

[jgonggrijp]

This is not a request for change (you can leave the code as is), but I want to show you an alternative approach that avoids both the deep levels of nested indentation and the relatively new syntax: spread, optional chaining and nullish coalescing. Such new syntax leads to expensive polyfills in the generated bundle. I will be using Underscore which, believe it or not, would have a much smaller footprint in the bundle than the polyfills.

The alternative approach starts by lifting the sharedProblemResponse constant to module scope. We also outfactor a few nested anonymous functions to module scope:

const withNullId = _.partial(_.defaults, {id: null});

function problemFeatures(response) {
    if (!response) return {};
    const problem = response.problem;
    if (!problem) return {};
    return {
        hypothesis: problem.hypothesis,
        premises: problem.premises,
        kbItems: _.map(problem.kbItems, withNullId),
    };
}

function extendProblem(baseParam, response) {
    return _.defaults(problemFeatures(response), {
        id: null,
        base: baseParam,
        hypothesis: '',
        dataset: Dataset.USER,
        premises: [],
        entailmentLabel: EntailmentLabel.UNKNOWN,
        extraData: null,
        kbItems: [],
    }, sharedProblemResponse);
}

Now we can simplify the code here to this:

Suggested change

	if (baseParam !== null) {
	return this.existingProblem$(baseParam.toString()).pipe(map(response => {
	const problem = response?.problem;
	return {
	...sharedProblemResponse,
	problem: {
	id: null,
	base: baseParam,
	hypothesis: problem?.hypothesis ?? "",
	dataset: Dataset.USER,
	premises: problem?.premises ?? [],
	entailmentLabel: EntailmentLabel.UNKNOWN,
	extraData: null,
	// KB items are not shared across problems.
	kbItems: problem?.kbItems.map(kbItem => ({
	...kbItem,
	id: null,
	})) ?? []
	}
	};
	}));
	}
	if (baseParam !== null) {
	return this.existingProblem$(baseParam.toString())
	.pipe(map(_.partial(extendProblem, baseParam)));
	}

[XanderVertegaal]

Oh, this looks very elegant, and if it has a smaller footprint I'm all for it! <3

I notice I do need to get more used to stuff like .map(_.partial(...)) and _.defaults(...). Using nullish coalescing and spread syntax still come much more natural to me. 😅