Yomguithereal / baobab

JavaScript & TypeScript persistent and optionally immutable data tree with cursors.
MIT License
3.15k stars 117 forks source link

Invalid (according to `validate`) values keep attempting to commit #450

Open yotamDvir opened 8 years ago

yotamDvir commented 8 years ago

My validate throws an error when attempting to assign the value 2 to the foo leaf. Since I didn't set validationBehavior to notify, when attempting such an assignment, an error is thrown, and the tree gracefully rolls back to its previous state, as expected. However, from now on baobab will attempt this assignment on all other commits! So for example, if I later assign hi to bar, validate will again throw the error. Furthermore, if I am applying an increasing function repeatedly on foo, then the result is: 0, 1, 1 with an error, 3, 4... I expected: 0, 1, 1 with an error, 1 with an error, 1 with an error... It seems to intensionally keep track of invalid commits in order to prepend them to newer commits. Is this the desired behavior? I would think that if I set rollback, it should really roll back, not keep trying to assign the value. At least this is the behavior I require.

Yomguithereal commented 8 years ago

Hello @yotamDvir. This seems to be a bug indeed. Could you write a minimal use case reproducing the issue so I can fix it asap please?

yotamDvir commented 8 years ago

The script below reproduced the issue. Could have been a bit more minimal using the minified script, but it wasn't reproducing the same bug (see end of this comment).

import React from 'react';
import ReactDOM from 'react-dom';
import Baobab from 'baobab';

const tree = new Baobab({
    counterA: 0,
    counterB: 0,
}, {
    validate: function (prev, next, paths) {
        const affectedA = paths.reduce(
            (wasAffected, aPath) => wasAffected || aPath[0] === 'counterA',
            false);
        if (affectedA && next.counterA === 2) {
            throw new TypeError('Validation failed.');
        }
    },
    validationBehavior: 'rollback',
});

const inc = x => x+1;

const render = tree => {
    ReactDOM.render(
        <div>
            <button onClick={() => tree.select('counterA').apply(inc)}>counterA</button>
            <button onClick={() => tree.select('counterB').apply(inc)}>counterB</button>
            <p>Counter A holds {tree.get('counterA')}.</p>
            <p>Counter B holds {tree.get('counterB')}.</p>
        </div>,
        document.getElementById('root')
    );
};

tree.on('update', (e) => render(tree));
render(tree);

When trying to increase counterA to 2, it will throw and won't increase. In this state, trying to increase counterB will throw again (which means counterA is still an affectedPath) and change nothing. When trying to increase counterA once more, it will not throw and succeed in increasing to 3. Moreover, it will also batch perform all those increases we made earlier to counterB while it was throwing.

It just doesn't "forget" the invalid changes it tries to make.

As I mentioned, I tried to create this minimal example using the minified script, but there the problem was worse: the validation didn't even prevent the tree from changing. Is the minified script updated? This should probably be a separate issue though.