Data cycle within ember-changeset triggers invalidation-after-set assertion

[ef4]

While validating, a changeset can consume its own _errors property before writing to it. This addon marks _errors as tracked:

https://github.com/poteto/ember-changeset/blob/072f062711df3f7e73ba81a887274002a3c98242/addon/index.js#L37

But if you read and then write a tracked property during rendering, Ember will throw the dreaded:

You attempted to update _errors on changeset:[object Object], but it had already been used previously in the same computation.

assertion.

Here is a tiny app that crashes at boot: ef4/--ember-changeset-bug-repro@90a2a00

Both the read and the write happen on this line:

https://github.com/validated-changeset/validated-changeset/blob/a23494a9a481fe639d4e00aaffdfc9e54ea50cce/src/index.ts#L795

this[ERRORS] = this._deleteKey(ERRORS, key) as Errors<any>;

First, _deleteKey's implementation consumes this._errors, and then we try to immediately assign to it.

I suspect the same bug is also lurking in rollbackInvalid(), which does the same thing.

The fix here is to make sure _errors is always only an output to your computation, never an input. Recompute the whole _errors whenever it is needed without reference to the old value. And if you need caching, make caches that are not tracked.

[snewcomer]

@ef4 You are totally right. I think this necessitates https://github.com/pzuraq/tracked-built-ins - via TrackedObject. Do you have any other ideas? To determine a new value to set, I don't see a way without the old value.

[ef4]

I don't know exactly what you mean, in terms of where you'd put a tracked object. My first reaction is that I wouldn't expect that to help because the problem is not a lack of tracking of deep mutations.

[snewcomer]

To update this[ERRORS] (in this case, delete a key), we need to access this[ERRORS]. Moreover, we "reset" the entire object so Ember knows there is a change. To get this to work, we either need an object with all the properties tracked (we don't control the shape, so this isn't an option) or we use TrackedObject here so we can avoid the "reset", which I assume is causing this error.

Does this sound right?

[ef4]

No, my point is that there's no need to ever delete a key.

Validations all run together I think? So construct a whole new errors object, never reading from the old one.

Alternatively, if you need to not rerun some validations, cache their answers but in a new untracked property. They you still always construct a new errors object each time, but the construction reads from the cache and not the old errors object.

[snewcomer]

Great ideas! So there are two processes that happen with these "tracked" objects like _errors, _changes, and _content.

1. We generally add / remove errors per key. In the case of _errors, this is consumed by dependent public getters on the Changeset (get errors, get isValid) and the "reset" is to ensure anybody listening re-computes when a specific keys is updated.
2. There is an explicit validate function that runs it all together.

My inclination for num 1, TrackedObject would be our answer so those dependent getters are notified. For num 2, I agree with your approach of constructing a whole new one.

Very interested in solving this. Just want to make sure I understand your position fully.

[snewcomer]

@ef4 Does 1) make sense or am I misunderstanding something with regards to your point? (definitely value your feedback on this topic).

[ef4]

I think you're misunderstanding.

This code is already relying on assignment of this._errors to notify anybody about anything. The deep mutations inside _errors aren't tracked and there's no reason to make them tracked, because everything is already updating fine without that tracking, because you always reassign this._errors itself.

All of that is good and I don't think you should change it. So no TrackedObject needs to be involved.

To solve (1):

• have an internal cache that is not tracked that keeps track of all the currently validations that have run. You can safely read and write it as much as you want without risking any assertions, because it's untracked.
• whenever you need to update one validator, read from the cache and write to this._errors.

This allows you to still incrementally update validators, while keeping this._errors write-ony.

[snewcomer]

Ok thx for the explanation! I think I have resolved the issue I was dealing with.

whenever you need to update one validator,

Fix coming this week. You are correct.

Second, I incorrectly expanded your description of the error to this example that reads and writes. Accessors like isPristine rely on this property so it will re-run when changed; say a form that relies on this property when the user first "touches" the form. This isn't a data cycle issue because it is not during the "render" step. This was my misunderstanding.

Thanks for sticking with me on this!

[ef4]

This isn't a data cycle issue because it is not during the "render" step.

I don't think you can really guarantee which parts of your library will get called during render. It all depends how people use it.

It looks like _changes has the exact same requirements as _errors, since it's tracked and it's always updated via assignment.

[snewcomer]

You are right and good point. Luckily, in these basic cases (non initial render), I'm not able to force an error in the dummy app or our tests.

However, this is where I believe we would have to take a different path with the set** property on changeset workflows if we did come across an error. The tracked properties will have to be written to possibly in the same step the read happens - e.g. a user enters an input in a form and reads isPristine or some other property. This would happen via _setProperty. I haven't done it myself, but I have seen artificial delays added to avoid this issue.

[ef4]

However, this is where I believe we would have to take a different path with the set** property on changeset workflows if we did come across an error.

No, that's really not different.

The state the user sets is purely an input to the changeset. You would only ever read from it, never write it. The resulting validation state is purely an output of the changeset. You would only ever write to it, never read from it.

I have seen artificial delays added to avoid this issue.

Yes, please don't ever do this unless you're deliberately forcing a double render and know why you want a double render (measuring DOM is the typical reason).

[ef4]

Also keep in mind that write-then-read is totally fine, it's only read-then-write that is a problem.

(To think about why it's a problem, consider that reading a tracked property causes that tracked property to be an automatic dependency of the code you're running. If your code then changes that property, it just invalidated itself.)

[snewcomer]

To solve (1):

I truly want to solve this problem. However, whether it is my lack of experience or lack of creativity, I am having trouble seeing a path forward that doesn't result in a major change. I'll present my general observation first and then after go into some debugging steps.

General Observations

I believe we can partially fix the issue. But there exists a scenario that I don't think we can fix.

• Fixable - validate() should read from cache and write to _errors. This is an easy fix.
• Not fixable - To access model.errors AND cs.validate(), we necessarily need to read _errors and possibly write to it. The only way to avoid the Ember revalidation error completely for all cases is to read from ERRORS_CACHE in get errors. However, then we lose reactivity since that isn't a tracked object.

Overall, what we can do is solve the read-then-write for the validate() function specifically with a cache. However, if the user is also trying to read _errors as in your example, I'm not sure the library can prevent this. What do you think?

Detailed steps

Lets take your example with some of the guidance you have provided...

0. render template and lookup model property.
1. validate() is called and in case of an error, we will read _errorsCache and write _errors - this[ERRORS] = this._deleteKey(ERRORS_CACHE, key) in _handleValidation.
2. read model.errors property on the changeset for display in template. I'm not sure why this is showing up after the previous step.
3. read and then write Error.

The previous steps don't explicitly illustrate read-then-write (rather write then read??). I'm just laying it out as I see it in while debugging as shown in the screenshots below.

Screenshots in order to error

• Notice no error to console yet. About to write to _errors
  

• Next, access "errors" property on changeset. This looks to be the original access for this property
  

• Error
  

Let me know what you think!

[snewcomer]

I think I may have been somewhat trolled here...We are accessing a property while trying to set it :(

https://github.com/validated-changeset/validated-changeset/blob/750eb3e21a84b4fcb87501434e35f99df5a8ee60/src/index.ts#L1029

If I can find a way around this, perhaps our problem will be fully solved with a cache. Will update after some investigation.