pivotModel stored within a proxy wrapper for a relatedModel, accessible trough a virtual property as configured by the 'pivotKey' #669
Conversation
Changed: Pivot attribute on a model to an active lookup on the pivot table.
Changed: Pivot attribute on a model to an active lookup on the pivot table.
Changed: Pivot attribute on a model to an active lookup on the pivot table.
fixed Typo
fixed Typo
Fixed Typo
- a proxy between the relation with an active lookup for pivot attribute
adm-bome
changed the title
- for all model keys with the virtual pivot key
- empty pivot property on proxy models won't serialize - handle the pivot property as an relation instead of a value
Codecov Report
@@ Coverage Diff @@
## master #669 +/- ##
=========================================
Coverage ? 98.99%
=========================================
Files ? 48
Lines ? 1585
Branches ? 268
=========================================
Hits ? 1569
Misses ? 16
Partials ? 0
Continue to review full report at Codecov.
|
adm-bome
changed the title
|
Unfortunately I think we need to revise this approach without the use of Proxies, if at all possible. Perhaps we can look into hydration of models without reference? |
|
IE is completely EOL (all IE even IE 11) and even microsoft doesn't support them anymore (https://techcommunity.microsoft.com/t5/microsoft-365-blog/microsoft-365-apps-say-farewell-to-internet-explorer-11-and/ba-p/1591666) It always possible to use this polyfill which doesn't cover all the Proxy class features because it's impossible but covers some simple use cases (which is the only thing this PR uses) Otherwise it would be possible to remove Hydration of models without reference would introduce an even bigger breaking change (Because then === doesn't work) This is a very important issue and I believe the way to go for now is proxy which can be poly filled enough to support this PR |
|
The main concern is breaking the support paradigm in Vue 2. Vuex ORM was initially and will continue to be adopted with Vue 2 in mind which is the primary reason Proxy is not the preferred choice here (else it would make a lot of things easier). That being said, IE is not necessarily the driving factor in this decision hence why it won't be a topic worth entertaining here. What's more important is the reasoning for simulating a technology to solve a problem that can be solved in a more amicable fashion without hindering the line of support that already exists.
I'm not sure I follow. The key issue with pivots is that they are attached by reference. This is not an issue for 1:1, 1:m or m:1 relations because pivots can only bridge a child relation once. However, m:m relations can share the same child – and since they are preloaded, attached, then enumerated to update pivot data exclusively, the reference becomes an issue. One solution could be to rehydrate models before attaching. This is not exactly ideal in a benchmark competition because in almost all instances models are hydrated when queried (even internally) anyway. Re-hydration inevitably adds to the Nth degree. However, it doesn't require polyfilling, doesn't introduce breaking changes since it's out of the public scope, and doesn't impact support coverage for projects with existing targets. Does this make sense? |
|
So the problem is the not that the pivot is attached by reference but rather the entire model, so when you do a query, you will always get the same reference to your model, and this is what is causing the issue. Because whenever you fetch a model with a different pivot, the pivot is overwritten on all the models because of the reference So actually, looking at the PR, yes the proxy is not needed, since it's anyways creating a new instance of the PivotModel proxy, It could just be replaced by But in both cases you'd have a small breaking change, because now many to many models accessed through relations will now never have the same reference because they are new instances on every query fetch Some tests related to memory should be done as well, to make sure that it won't cause a memory leak |
|
Hah, quite right. Indeed I was referring to the model instances the pivots are attached to. Nonetheless, I think it’s safe to say we’re seeing the same picture now. Regarding object equality, I think for a m:m relations this would be quite difficult to maintain without substantial refactoring, particularly since a reference to hold existing models to prevent the breaking change would indeed lead to memory leaks. As mentioned in earlier issues you’ve referenced, the test suites didn’t cover the more complex scenarios so this was missed during development. Of course, we welcome PRs that resolve existing issues and this one has been a long standing issue. |
Actively look-up pivot data on models
Type of PR:
Breaking changes:
Details
Bug
Pivot data, can and 'almost' always, is different for the related relation to the model. (why else have pivot data).
When models have an relation to the a related Model, Pivot data should not be stored on the related model, because the related model is a object by reference the pivot information gets overwritten each time the mapper sets the pivot record to the related model. only the last mapping is remembered.
Solution
The related model should be a proxy between the related model and it pivot record. Pivot on the proxy must be a 'virtual' property.
Feature
Possibility to have multiple models with a relation to the same related model with different pivotData.
Refactor
from the pivotKey in a model, to an Proxy with an 'locally/virtual' stored reference to the pivot data.
Issue: #668