-
-
Notifications
You must be signed in to change notification settings - Fork 407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TrackedList #569
TrackedList #569
Conversation
|
||
```js | ||
class Person { | ||
@friends = ['Yehuda', 'Melanie', 'Ricardo']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@friends = ['Yehuda', 'Melanie', 'Ricardo']; | |
@tracked friends = ['Yehuda', 'Melanie', 'Ricardo']; |
import { A } from '@ember/array'; | ||
|
||
class Person { | ||
@friends = A(['Yehuda', 'Melanie', 'Ricardo']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@friends = A(['Yehuda', 'Melanie', 'Ricardo']); | |
friends = A(['Yehuda', 'Melanie', 'Ricardo']); |
} | ||
``` | ||
|
||
## Motivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add to this that there's another (and IMO even more important than paradigm-preference) reason that this is valuable: performance. Immutable functional updates have many advantages, but without something like persistent data structures, they can carry a high performance penalty.
TrackedList
allows users to make that tradeoff as they like, whatever their programming paradigm preference—and there's no reason that you can't provide a functional abstraction around it if that's your jam.
prototype, which is very bad, or on re-implementing most array methods under | ||
the hood. | ||
|
||
### Introducing `TrackedList` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern I have with this name is that List
suggests, well, lists, which are a standard abstraction with specific characteristics. At a minimum we will need to document how this List
type is implemented, so that people will have a sense of the performance characteristics – and specifically, if this is implemented on top of JS arrays rather than as a linked list, we should be explicit about that (presumably adding a note to that effect in the How We Teach This section).
} | ||
``` | ||
|
||
### `sort` and `reverse` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Borrowing an idea from Swift, perhaps we should name these sorted
and reversed
, the difference of names implying the difference in outcome?
version of native arrays, where `TrackedList` implies that it is its own data | ||
structure, with potentially divergent behaviors. | ||
|
||
## Detailed design |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great if we included the full API spec in this section!
|
||
## Unresolved questions | ||
|
||
- How does this impact the potential functional array-methods RFC (functional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops: incomplete sentence!
place. This would limit the interoperability of `TrackedList` somewhat, but | ||
would likely not have a massive impact. | ||
|
||
## Unresolved questions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is TrackedArray
intended to be subclassed? If so, what invariants do subclasses need to uphold?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had exactly this question.
Also, what is the intended API for a subclass to manually invalidate itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TrackedList
is not intended to be subclassed, though there isn't a way to prevent it in JS, and there is no API for manual invalidation. If users want to implement a custom iterable, the recommendation would instead be to use the standard Symbol.iterable
API to do so, as it should be fully supported by Ember at this point. If there are still some gaps, they should probably be addressed in a separate RFC.
|
||
### `length` | ||
|
||
`length` will be an immutable/readonly property on `TrackedList`. This is to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`length` will be an immutable/readonly property on `TrackedList`. This is to | |
`length` will be a readonly property on `TrackedList`. This is to |
I think if I'm understanding the intent here correctly, we should describe this as readonly
but not immutable, since it will change if users push
or pop
from the list. (With immutable data, I'd expect that the value will never change in the life of the instance; the only way to get a new value is to get a new instance.)
Also, is length
auto-tracked? (I'd assume yes, but I'd love if it were explicit here.)
`TrackedList` will be as close to 1-1 with the native array API as possible. | ||
However, it will not attempt to be a drop-in replacement for native arrays, | ||
since this is not possible without native `Proxy` (which is not supported in | ||
older browsers) and would have major performance caveats even if it were. This | ||
is also the motivation behind the name: `TrackedArray` would imply a tracked | ||
version of native arrays, where `TrackedList` implies that it is its own data | ||
structure, with potentially divergent behaviors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions as I'm considering this:
- What would those "potentially divergent behaviors be"?
- Is it possible to ship a
TrackedArray
now, with future performance improvements unlocked when IE11 is removed (whenever Ember 4 ships), and if so, how do the tradeoffs of doing that compare to the tradeoffs of introducing this class now and then potentially another class later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Specifically, the two divergences in this RFC are:
- The difference in behaviors of
sort
andreverse
- The readonly nature of
length
- The difference in behaviors of
- It is not possible to ship
TrackedArray
without native Proxy, so we would have to wait for IE11 support. The native Proxy version is still much much slower than a native array based solution, by at least an order of magnitude, currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarifications!
Here's something to maybe argue about here, or maybe just to add to the drawbacks section (or as a sub-point of the first drawback). I'm generally not a fan of object APIs that look really really like standard/native object APIs, but differ slightly. All too often I find one of the significant challenges in reading and understanding code in dynamically typed languages is figuring out what a particular thing is. Good documentation or, better yet, typescript should largely ameliorate this, but it's still a significant factor. So I'm not a huge fan of implementing something that looks really really like a native array, but is missing one or two pieces ( function doSomeStuff(items) {
let things = items.map((i) => i.getThing());
for (let thing of things) {
thing.doStuff();
}
} and maybe change it to function doSomeStuff(items) {
items[0].getThing().doStuff();
let things = items.slice(1).map((i) => i.getThing());
for (let thing of things) {
thing.doDifferentStuff();
}
} except whoops, I'm kinda ambivalent because I'm not sure I'd really prefer a situation where all the methods do the same thing as |
Can't we use kinda: const impl = Symbol();
function defineKey(ctx, key) {
if (key in ctx) {
return;
}
Object.defineProperty(ctx, String(key), {
enumerable: false,
configurable: true,
get() {
return ctx[impl][key];
},
set(value) {
ctx[impl][key] = value;
defineKey(ctx, key + 1);
}
})
}
class MyArray {
constructor(orig = []) {
Object.setPrototypeOf(this, Array.prototype);
this[impl] = orig;
defineKey(this, 0);
Object.defineProperty(this, 'length', {
enumerable: false,
get() {
return this[impl].length;
},
set(_) {
// noop
}
});
return this;
}
} Writing to "uninitialized" index of array - it's really "corner" case and we can skipt it at all, mention it as anti-pattern in documentation. |
@lifeart that would be very inefficient, and a leaky abstraction since there is no way to know when a value has been initialized. At that point it would be better to wait for and use native Proxy. @bendemboski I understand the concerns you've raised, and they're totally valid. Unfortunately, we cannot make a fully transparent array class at the moment without native Proxy, and even when it becomes possible it may not be performant enough for some use cases (my own naive micro-benchmarks show that native arrays are still 100-times faster than a naive Proxy wrapper). Personally, I feel like We also can use native proxy in development, so for instance if you tried to do: let first = trackedList[0]; We can throw an error to the user and let them know they need to use |
@pzuraq why |
let arr = [];
arr[0] = 123; Is valid, and not possible to instrument without Proxy. It would mean that you cannot transparently pass the array around, especially in non Ember code, because some other method would have to be used to setup the (extremely expensive) native getter and setter. |
do we really have such performance problem? (in real life apps)? We trying to create conceptually strong solution, instead "usable". But it don't need for 80% of app developers. Maybe it's make sense to create "slow" array structures (convenient with classic arrays and well-known for all js developers), and "fast" one for specific usage (bottlenecks) with different APIs? |
we can use Babel and replace // svelte transforms |
I’m personally more worried about the leaky abstraction than the performance problem. IMO, any solution that attempts to use native get/set syntax must be able to do so perfectly, so we must wait for native Proxy. Users who don’t need to support IE11 today can of course use native proxies to create an array wrapper as is. For an official Ember solution, I think the options are to continue using EmberArray until we drop support for IE11, or to pursue something like TrackedList. |
Let me rephrase: let arr = new MyArray([]);
arr[0] = 123; Is also impossible to instrument. It is impossible to do this for any and all JS objects without native Proxy. |
@pzuraq possible #569 (comment) In software development there is no perfect solution, it's always balance between simplicity, performance, maintainability, extensibility. I'm afraid TrackList may become one more "Ember Array" for next years, instead of not ideal, but widely usable and well-known property patching (vue js) |
@lifeart that abstraction is still leaky, which is my overall point. There may not be perfect solutions, but there are correct solutions, and native Proxy is the only one that is correct in the way you want it to be. I don't believe we can ship anything less correct.
This has been a continual problem in our design process, that we let the perfect be the enemy of the good. The point of this proposal is to make things incrementally better. Personally, I want a slightly better Ember today much more than a much better Ember a year from now. We may just have to agree to disagree on that. |
Is it possible to ignore // react users living with it pretty well all we need - offer devs use ramda or kinda library to deal with immutable arrays, instead of supporting N array apis internally. |
The immutable pattern is fully supported, and we definitely should mention it in the guides and have an In Depth guide on its usage. |
The goal of `TrackedList` is to provide a better `EmberArray`, in the short | ||
term, for Ember users who prefer this style of programming; One that lines up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RFC should probably mention, if only as an aside, what the long term preference is, if not TrackedList
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably it's Proxy
, but it's not totally obvious since later on when Proxy
is discussed a couple of important caveats are emphasized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, if this is not the long term solution, it should not be added to core and should remain in an addon.
I understand that this can be helpful for people who prefer the pre-Octane way of dealing with mutable/tracked (Ember) arrays but isn't the fact that So while I'm not completely opposed to this, the above is just the first thought I had when seeing this. Would it be an option to release this in an addon if we want it at all and not make it part of standard Ember? The same comment applies to #577 of course. |
@marcoow yeah, I definitely understand that and agree that in general we want to make the programming model really feel like it's "Just JavaScript" still, and any extra layering on top of vanilla JS should be carefully considered and thought through, since it'll add cognitive overhead. For context, the guides and official recommendations in Octane still all use
This is not to say that it would be completely off the table to change the guides and recommendations to use this style, it's just that these patterns need to be figured out. Additionally, potential future JS features like Records and Tuples would make this style much nicer and possibly solve many of the concerns listed above. Libraries like Immer exist today, but I feel like we would have to be very confident in them to recommend them by default. I suppose from a personal perspective, I really started to feel like this was the right solution as I began to work more with autotracking and think through its principles, from a theory perspective. The fundamental assumption that autotracking makes is that all root state that is input to the system is tracked. Currently, we only expose ways to tracked specific properties by default, but mutable collections like arrays generally exist, and we want to track changes to them as well. The current solution feels like it's breaking that principle by asking you to notify on something unrelated when you mutate an array, map, or set. FWIW, I'm really excited about Records and Tuples though and could really see them be used this way. Since they're immutable, they don't introduce any of these problems, which is perfect! I could also definitely see a guide for using Immer in Ember apps. |
As exciting and cool as this is, I think the focus of 2020 should be to make the surface area of Ember smaller, not bigger. In real world scenarios, the tradeoff at PR review time is to either nudge the author of code towards understanding the reactivity model and its caveats or using I would like to see this shipped as a library completely agnostic of Ember though (not an Ember addon) and let users opt in. |
@pzuraq thanks for the in-depth reply. I understand there are pitfalls with the immutable model and agree there needs to be a way to get around those. I think I'm just worried (similar to what @mehulkar is saying) that as we're now in a situation where |
@mehulkar @marcoow I definitely understand these concerns. I think the current state of things is though that we haven't gotten rid of those extra wrappers. Specifically, we currently cannot deprecate or remove We can come up with alternative solutions in the community, but until we can get consensus on an alternative default for arrays/lists, I believe |
I personally like that auto-tracking is just limited to classes and the root level properties. In the past I had situations where a But, I agree that the auto-tracking system is not very ergonomically when it comes to arrays, POJOs and other non-trackable types. The current example in the guides shows that first hand: class SimpleCache {
@tracked _cache = {};
set(key, value) {
this._cache[key] = value;
// trigger an update
this._cache = this._cache;
}
get(key) {
return this._cache[key];
}
} Yet, I belief there are other solutions that could be explored here. A quick idea of the back of my head would be to provide a magic class SimpleCache {
@tracked _cache = {};
set(key, value) {
this.tracked._cache[key] = value;
}
get(key) {
return this._cache[key];
}
} Maybe with something like that there is no need for |
Great proposal overall! I think designing something proxy-based would be much better and browser support is excellent. Let's come up with something modern, with longer shelf-life. IE 11 is in rapid decline and only have a few percentage points of usage these days. Many large software companies has already stopped supporting it, for example:
The days where IE11 was the only browser installed on a computer are long gone. Ember should drop IE 11 support and focus time on using the modern JS toolset to provide a nice framework. Any Ember user that need to extend support a while longer, e.g. end of 2020, could simply linger on an Ember LTS a while longer. It doesn't need to hold back new framework development. Regarding the performance drawback with proxies, I don't think it would be a big problem under most real-world scenarios. |
Could we not I dont like the current way no notify a change |
@luxferresum there are two main issues I see with that potential solution:
This is actually not how we expect most users to have to work with external libraries. Autotracking is a very flexible reactivity system, and is designed to allow libraries to be wrapped transparently. In general, manually entangling and instrumenting notifications from external libraries is something that should be done once in a wrapper library, and then within Ember apps it should be consumable without having to worry about the details of how the library autotracks. The assumption should be that if the library mutates state, it also correctly entangles and notifies that state. For examples of what this looks like in action, we can look at Ember Data and tracked-built-ins. In the case of Ember Data, it provides the This same strategy could be applied to, say, Apollo, Redux, or any number of other external libraries. We are working on stabilizing autotracking APIs so that instrumenting libraries is easier and the community can begin experimenting more easily, but I definitely think something we want to avoid is manually notifying for every single external library, every time it is used in an app. How to do it should be well documented and understood though, it's not that these wrapper layers would provide "magic", they would just would place the entanglement where it logically belongs - with the source of the data. |
How does this compare to https://github.com/tracked-tools/tracked-built-ins? |
@wagenet Awesome with all the RFC cleanup lately! 🎉 |
Rendered