Closed ef4 closed 3 years ago
@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.
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.
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?
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.
Great ideas! So there are two processes that happen with these "tracked" objects like _errors
, _changes
, and _content
.
_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.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.
@ef4 Does 1) make sense or am I misunderstanding something with regards to your point? (definitely value your feedback on this topic).
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):
this._errors
.This allows you to still incrementally update validators, while keeping this._errors
write-ony.
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!
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.
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.
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).
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.)
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.
I believe we can partially fix the issue. But there exists a scenario that I don't think we can fix.
validate()
should read from cache and write to _errors
. This is an easy fix.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?
Lets take your example with some of the guidance you have provided...
model
property. 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
.model.errors
property on the changeset for display in template. I'm not sure why this is showing up after the previous step.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.
_errors
Next, access "errors" property on changeset. This looks to be the original access for this property
Error
Let me know what you think!
I think I may have been somewhat trolled here...We are accessing a property while trying to set it :(
If I can find a way around this, perhaps our problem will be fully solved with a cache. Will update after some investigation.
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:
assertion.
Here is a tiny app that crashes at boot: https://github.com/ef4/--ember-changeset-bug-repro/commit/90a2a00cfc109f0bda5cf9dd4793532e173cb0c0
Both the read and the write happen on this line:
https://github.com/validated-changeset/validated-changeset/blob/a23494a9a481fe639d4e00aaffdfc9e54ea50cce/src/index.ts#L795
First,
_deleteKey
's implementation consumesthis._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.