Unwanted declaration merging

[pcafstockf]

What a great project!
When I stumbled upon it, I actually went looking for somewhere to use it. 🥇
The way you use JavaScript features to make the rules feel more natural, is very cool.

The premise merging feature is a great optimization... except when it's unwanted :-)
Disclaimer: I'm not an expert with rules engines, so feel free to point out if my thinking about this all wrong.

Consider a (somewhat contrived) example of a collection of rules which each ensure that a required section of a dynamic document are present:

requiredSections.forEach((clazz) => {
	rules.push(new Rule({
		name: 'require ' + clazz,
		when: [
			(facts) => (! document.querySelector('div.' + clazz))
		],
		then: (facts) => {
			appendSectionTemplate(sectionTemplates.get(clazz));
		}
	}));
})

You could have 8 required sections, but they will all hash to the same premise, and so only one of the 8 actions will fire, ever.

If I've not missed something and this is a valid concern, what about enhancing the API such that a premise could be a function or an array the first element of which would always be a function.
Something like:
[(facts) => (! document.querySelector('div.' + clazz)), clazz]

This would still allow for hashing of the premise, would not break any existing rules/code, and would only require changes to 2 lines of code.

RuleSet.register would need a 1 line change:

        premise = new Premise({
          ...rule,
          id: this.nextPremiseId(),
          name: `${rule.name} / ${index}`,
          when: Array.isArray(when) ? when[0] : when,
        });

And to keep bad things from happening, the assert on line 36 of Rule.js would need to be:
this.when.reduce((acc, premise) => acc && (isFunction(premise) || (Array.isArray(premise) && isFunction(premise[0]))), true),

This would provide premise merging by default, but allow an override on an as needed basis.

If this sounds interesting, I would be happy to provide a PR.

Thoughts?

[frankthelen]

You have a point.

I have created a Dos and Don'ts section with your example.

I think it boils down to the question whether premises (when functions) are "pure functions" or not. A "pure function" being a function that only relies on its function parameters (i.e., the facts) and does not contain any non-local variable references.

Which brings me to:

• Pure when functions -- Strongly recommended.
• Non-pure when functions -- Possible if we find a way to flag them appropriately (e.g., with isPure=false). But still, it is required that all outside value references are static/stable during an evaluation run. If not, the result becomes unpredictable.

[frankthelen]

I added some additional explanation to the readme how premises should look like.

[pcafstockf]

I readily admit to not fully grasping the internals of rools, but my understanding of your explanation in the ReadMe is that...

This is good:

rules.push(new Rule({
    name: `Rule evaluating 5`,
    when: (facts) => facts.foo >= 5
    then: (facts) => // ...
  }));
rules.push(new Rule({
    name: `Rule evaluating 8',
    when: (facts) => facts.foo >= 8,
    then: (facts) => // ...
  }));

But this is bad:

const createRule = (value) => {
  rules.push(new Rule({
    name: `Rule evaluating ${value}`,
    when: (facts) => facts.foo >= value,
    then: (facts) => // ...
  }));
};
createRule(5);
const other = 8;
createRule(other);

It appears to my novice eyes that both implementations should be valid.
However, the sanity check to ensure no duplicate premises (md5 hash), gets in the way of the second implementation.
My proposed patch was just to allow any user who was confident they did not have duplicate premises, to force generation of a different md5 hash.
This is important, because it is just not always possible to hardcode premise values. We need the ability to generate a rule, and accept the responsibility to guarantee that the premises are all unique.

Just to clarify for anyone else reading this, I'm pretty sure what @frankthelen is saying above is that the following implementation would create unpredictable results because cbFn() returns varying values.
The premise must remain stable (which in my second implementation above, it would).

const createRule = (cbFn) => {
  rules.push(new Rule({
    name: `Rule evaluating callback`,
    when: (facts) => facts.foo >= cbFn(),
    then: (facts) => // ...
  }));
};
createRule(() => Date.now());

[frankthelen]

I agree.
Your "But this is bad" example above doesn't work because writing...

createRule(5);
createRule(8);

...still lets Rools think that their premise facts.foo >= value is identical and therefore can be optimized.
But I agree that creating rules like this should be possible.
Limiting it to "pure" functions (and functions with global variables) is probably too limited.
Though I still believe that using "pure" functions that only refer to the facts without any other "external" references are very desirable because the most predictable scenario.
Anyway. What are the options then?

• Option 1 -- Just forget about the idea of optimizing premises and treat each premise as unique.
• Option 2 -- Limit Rools to "pure" functions (and functions with global variable). As is today.
• Option 3 -- Find a way in between, e.g., support both, "pure" functions (open for optimization) and non-pure functions (to be treated as unique and not to be optimized). Maybe something like when vs whenPure would already do the job. If you only use when, ok, no optimization. But you can use whenPure to allow optimizing duplicate premises.

[pcafstockf]

My 2 cents (for all that's worth) is option 3 feels more like rools. But it does break backwards compatibility by inverting when optimization would happen. Not a big deal for me, but perhaps for other folks.

I like your original thought of maybe adding isPure=true as a construction parameter to RuleSet. If true (or even undefined), you initialize premisesByHash as it does today. But if explicitly false, you leave premisesByHash undefined. Then the registration function can easily tell if it should optimize or not for that particular RuleSet.

Keeps backwards compatibility, encourages "pure" function implementations by default, and allows a simple escape hatch when needed.

[frankthelen]

The idea of option 3 was to create a rule like this (Option 3a):

const rule = new Rule({
   when: ... // like today -- but without optimization
   whenPure: ... // explicitly encourage optimization
   then: ...
});

It is backward compatible in the sense that existing rules still work -- plus it fixes yet unsupported rules. It is more like a fix.

We could also inverse the logic (Option 3b).

const rule = new Rule({
   when: ... // like today -- optimization is the default
   whenNonPure: ... // explicitly without optimization
   then: ...
});

It is backward compatible in the sense that it adds support for non-pure premises whilst leaving existing functionality as is.

The original thought of flagging single premises (Option 3c) "feels" a bit cumbersome to me. Something like this:

const rule = new Rule({
   when: [
      (facts) => facts.foo.bar === 0,
      { premise: (facts) => { ... }, pure: false }, // explicitly without optimization 
   ],
});

Doing it on rule level is a bit more restrictive but possible as well (Option 3d), e.g.:

const rule = new Rule({
   when: ...
   whenPure: false, // explicitly w/o optimization -- but for all premises of this rule
)};

Don't like that too much because the property is conceptional at the wrong location.

[pcafstockf]

Personally I like 3b, but would be pleased to see any of those (3a,3b,3c, 3d) implemented.

[frankthelen]

Cool. Will find some time. Thanks.