hasInitialValue_ should return false if value is undefined, but validators should run regardless. Fixes #259

[robframpton]

No description provided.

[jbalsas]

Hey @Robert-Frampton, do we really need all three tests in this PR? Aren't they all testing over the same source code, or is there some integration point worth re-testing?

[robframpton]

Hey @jbalsas

I'm fairly certain we could just keep the test in metal-state, I just liked the idea of having tests that also cover the data managers since they are a little different from one another.

I'm honestly fine either way, do you wanna remove them?

[eduardolundgren]

Are we sure about this change? Having initial value means that the user passed a value, even if it is undefined. This changes the behavior of the functionality.

[jbalsas]

This wasn't the original behaviour, if I understood @Robert-Frampton correctly in this comment, which points to the current behaviour to be unintended...

I did some digging, and this behavior is an unintended side effect of f1e8847. That fix intended to ensure that validators are run for undefined values, in case isRequired was being used. However, it also makes it so that undefined will overwrite default values.

I ran @bryceosterhaus' tests against the commit right before f1e8847 and it passes.

Since this hasn't always been the behavior, I think we should go ahead and fix it. We can still run it against some larger code bases though, just to be safe.

We just didn't have a test back then that would've caught the change...

[eduardolundgren]

@jbalsas we had this conversation by that time, and the topic was if we should consider that a passed undefined value is considered a value or not, in any case, the change makes sense indeed.

[jbalsas]

I just agreed because @Robert-Frampton found out that was the old behaviour.

I would defend that this is actually wrong and unintended...

initialConfig = {
    foo: undefined
};

ComponentDataManager.setUp(component, {
    foo: {
        value: 'defaultFoo'
    }
});

assert.strictEqual(component.foo, 'defaultFoo'); // I would really expect it to be undefined

[eduardolundgren]

Uhh, that's odd, if it was like this before, and now we have it different for few months, return to the old behavior might break users' code.

Let's move with the fix for the regression, we have to deal with the breaking change.

Thank you for clarifying it!

[jbalsas]

Hey @Robert-Frampton, I'm fine keeping the tests for now, under the assumption that we want to verify that all the managers work that way and are not allowed to overwrite this behaviour.

I'm merging to develop! 👍