-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Add tracked-built-ins
dependency
#812
Conversation
|
||
## Unresolved questions | ||
|
||
- Should we introduce these at something like `@glimmer/tracking` instead? |
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.
This'd be my preference.
Also, providing an optional deepTracked
for 🎵 folks that just don't care 🎵
(though, the split out imports is fine by me, too)
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.
@NullVoxPopuli interesting note! I think it's probably fair to say that that definitely won't happen as part of this RFC. It may be worth adding, but needs its own RFC, because there are non-trivial design and teaching questions about what it looks like to include that.
(My own take, not necessarily shared by anyone else on the Framework or TS core teams, is that using it is an anti-pattern outside of tests, usually reflects some other gap even in tests, and that there are generally much better ways of solving the needs people reach for that pattern for.)
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 agree lol)
whenever I've needed deep tracking in practice, I've always found it easier to recursively use the cache primitives, so I can select what is tracked as I recurse
- Should we introduce these at something like `@glimmer/tracking` instead? | ||
|
||
```js | ||
import { tracked, TrackedMap, TrackedSet /* etc. */ } from '@glimmer/tracking'; |
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.
Maybe a stupid question, but do we need the TrackedMap
, TrackedSet
, ... exports if we can do tracked(Map)
, tracked(Set)
, ...? To me, it seems nice to only have import { tracked } from '@glimmer/tracking';
.
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.
Hum, that fells less intuitive though. In practice I've always used the imports instead of wrapping.
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 wrapping implies -- "well, what else can I wrap"?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Could also see it as a decorator
@tracked(Set)
ids: Set<number>
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.
Given that tracked
has to operate differently for each type of thing I don't think it is a great idea to overload its signature.
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.
Yeah, probably right
or using `tracked()` decorator: | ||
|
||
```ts | ||
import { tracked } from 'tracked-built-ins'; | ||
|
||
class Foo { | ||
@tracked value = 123; | ||
|
||
obj = tracked<{[string: number]}>({ foo: 123 }); | ||
arr = tracked<string, number>(['foo', 123]); | ||
map = tracked<string, number>(new Map(['foo', 123])); | ||
set = tracked<string, number>(new Set(['foo', 123])); | ||
weakMap = tracked<{[string: number]}>(new WeakMap([[{}, 456]])); | ||
weakSet = tracked<string, number>(new WeakSet(['foo', 123])); | ||
} | ||
``` |
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.
Raising this explicitly for discussion: I do not think we should include this part of the tracked-built-ins
API.
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 agree with this. In practice, I've found that importing the decorator from tracked-built-ins
gets confusing. In some components, you'll import it from @glimmer/tracking
, and in others you might use tracked-built-ins
instead. I find it much clearer to import the classes and instantiate them instead. We've even found rare cases where we need to use both:
@tracked someMap = new TrackedMap();
Being able to consistently understand what @tracked
is capable of no matter which file you're looking at without having to check where it's imported from is much better for developer ergonomics, IMO.
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 tend to agree with this - tracked()
function may be misleading and you need to be careful what you use and where.
would be puzzle to have in the file both tracked
provided by @glimmer/tracking
and tracked
provided by tracked-built-ins
as you would have to rename one of those in the import statement
In some cases however, there could be a prohibitive number of possible properties, | ||
or there could be no way to know them in advance. | ||
In this case, you should use tracked versions of JavaScript's built-in | ||
Plain Old JavaScript Object (POJO), array or keyed collection (Maps, Sets, WeakMaps, WeakSets) |
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.
should we explain why certain structures are or aren't supported?
Like, I have some projects that make heavy use of Uint32Array. I'm sure other folks have other stuff they'd like tracked versions of (or a way to optimally track them)
Status: this was on the agenda for this week’s Framework Core Team meeting, but we had a couple of longer discussions about other open RFCs before we got to it. I expect we’ll be able to cover it next week. |
Thanks for writing this RFC. I ever wanted tracked built-ins from the get-go of However, since then auto-tracking has made really great improvements on the technical implementations to track "more things" (which is why I like this RFC) - however on the API surface, this has led to severe problems, and I'm afraid this RFC will not solve but instead promote them. I gonna explain them as a story I've heard way to often (only just this year) and then provide coniderations for the API surface. A Story About
|
I echo everything @gossi said, and from fielding questions from dozens of new developers at work (both new in general, and more experienced, but net to ember), the api surface bloat in absolutely a problem. And merging the addons seems like a good way forward. import { tracked } from '@glimmer/tracking';
class Demo {
@tracked num = 2;
@tracked str = '2';
@tracked({ shallow: true }) array = [];
@tracked({ shallow: true }) obj = {};
@tracked({ shallow: true }) array = new Map();
@tracked({ shallow: true }) aset = new Set();
@tracked({ shallow: true }) weekarray = new WeakMap();
@tracked({ shallow: true }) weakset = new WeakSet();
@trackkd({ deep: true }) deep = [{ }]
} This should be doable in a non-breaking way with the existing |
FWIW I disagree fairly strongly that deep-tracking should be a built-in capability or especially a default, but that's my own take only, and I’ll be curious to see what other folks from Framework, Learning, TypeScript, etc. think as they look at this and chime in. I think it’s important to keep in mind that one of the goals of auto-tracking as a reactivity system is to be as small a layer over normal JS as possible. All of the things you pointed to are real things to learn, but they are things people need to learn about JavaScript’s semantics. If you want to track a Having small friction points or bumps can actually be really useful pedagogically, because they can help people. Our goal is not to have people avoid having to think about these things at all; it's to have them able to think about them at the appropriate level of granularity. It is of course possible to reasonably differ on what that level of granularity is, but it's not a given that “the most exhaustive end-to-end implementation” is the right spot in the design space. |
I 100% agree with this, but I don't see the decorator as the reactivity primitive. It's a DX abstraction, and should focus on improving DX around the primitives (which is what @gossi and I's comments are really trying to arrive at).
how so? of the non-OSS folks I've talked to, 0 folks have thought what we have today is a great situation tracking objects and arrays, and creating dynamic buckets of tracked data is one of the most common questions from new starters. And since we have a potential to provide an obvious solution that feels good, I think we should explore it:
I'm 100% here on the default, and I'm only 50% there on built-in. It's fairly trivial to build, but there are a number of things that folks have tried to use deep tracking for that just aren't possible -- and I don't want folks to get bitten by that. |
I'm not sure my 👍 adequately conveys how much I agree with @gossi here. So let this by even more of a 👍 My ideal is that |
I found firsthand these pain points when I was transitioning from vue/react to Ember. It was the exact same path- where I found that I had to use other tracked imports to get what I wanted done done, or use some unsatisfying I would really love to see |
Where does this mental model come from? let array = [];
array.push('foo'); // array has just been mutated Because to me array has not been mutated there. The underlying data structure (elements within the object have mutated but the variable array remains the same. The by-reference mental model harks back to the C days and has remained in programming languages through PHP, Ruby, Python, Java, etc. If we want the construct that mutating the encapsulated data within an object to mean mutating the reference itself then we are talking about immutable style code which is what immer.js is for. React and Vue have the advantage of a virtual DOM allowing every mutation to be expressed as an immutable (pure) function (build the whole thing in one go). By contrast Ember cherry picks its updates based on what things it knows have been consumed and then mutated. Thus the mental models we have in the React mind set are not the same when in the Ember mindset and I think that should be okay. I will admit there is some sugar (and performance improvements) to the idea of What I have found lacking is having a clean way to express consumption within my own classes. I often make classes to encapsulate my own data and within might use alternative primitives that are not as ergonomic when the only exposed API is |
for clarification on my stuff - I'm 100% in camp performant by default (which is just references) -- but I don't think that has to be at odds with the additional behavior(s) |
Agree with performant by default, but the framework should provide a comprehensive toolset so developers won't be frustrated when they realize there are no built-in solutions. Developers definitely will be encouraged and feel satisfied when they see a complete API around the reactive system; take Vue.js as an example: No one would complain about too many features a swiss knife has, as long as they've been designed correctly and adequately. also, people should learn and understand the nature of language itself to choose the right tool for the right task. The framework, on one side, provides the whole toolset to prove its powerfulness and flexibility. On the other hand, offers high-quality guides and other materials to teach users how to use them. These two aspects are not mutually exclusive. |
👋 I started using Ember about a year ago and used React before. I do have quite some experience with React and still use it for my biggest side hustle (Planubo). The tracked property was one thing that confused me since Ember does not handle it out of the box like React did. After React switched to hooks, it took a while for most folks to find out what exactly is needed, but once there, it provides a bunch of things like
☝️ Yes please 🙏 From @NullVoxPopuli
☝️ Can relate. Happened to me. 💯 From @NullVoxPopuli @tracked num = 2;
@tracked str = '2';
@tracked({ shallow: true }) array = [];
@tracked({ shallow: true }) obj = {};
@tracked({ shallow: true }) array = new Map();
@tracked({ shallow: true }) aset = new Set();
@tracked({ shallow: true }) weekarray = new WeakMap();
@tracked({ shallow: true }) weakset = new WeakSet();
@tracked({ deep: true }) deep = [{ }] ☝️ 🤤 |
Notably That does not by itself determine whether we should also add further capabilities in the form of a deep-tracked variant of the |
I do not think comparing our stuff to immutability APIs does us any favors though because we encourage mutability, so we inherently must consider different ergonomics. (I'm still 🤷 about the deep tracking, btw -- more just pointing this out as something to consider -- we don't want to base decisions off comparisons to other framework's utilities (but it is totally fine (and encouraged) to be inspired by them!)) |
@NullVoxPopuli I addressed that explicitly by noting that we do in fact already provide support for common mutable data structures. Deep-tracking is its own ball of wax that goes very far beyond just having good support for mutability. That, again, is not getting into whether or not it’s worth having available with For example, even if we choose to support deep tracking out of the box, having it come as a different import (rather than options on the main |
I put this on the agenda for today’s spec group meeting (spec group == breakout session from the Framework Team meeting, where we deep dive on technical details of things, which other relevant folks are invited to as makes sense!) so we can hopefully make some progress on it! |
We did not get to this today (you can see notes on today’s meeting here; we covered a lot, but didn’t make it to this)—but it’s at the top of the list for next week’s meeting! |
At our last Framework team discussion of this, we tentatively concluded that the one area we might want to leave open the idea that constrain ourselves to match the public APIs except for constructors, since (e.g.) On deep tracking, we noted that we think it's a valuable feature but (a) not actually obvious exactly what the right semantics should be, (b) definitely not viable as part of As for (b) and the proposals above, consider—folks here have suggested this should work out of the box (as indeed it does today with tracked-built-ins): let counter = tracked({ count: 0 }); Folks have also suggested that one of these should work: @tracked({ shallow: true }) someObj;
@tracked({ deep: true }) someObj; These are mutually exclusive. Either Footnotes
|
Co-authored-by: Chris Krycho <[email protected]>
77f1dce
to
38856c8
Compare
- add note about shallow copy - add note about missing deep tracking
38856c8
to
401a58b
Compare
sorry for the delay here
added the requested tweak in commit ca8525d
added the requested tweaks in commit 401a58b @chriskrycho @rwjblue please let me know if this does not match exactly what you were thinking/proposing. |
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.
These changes seem good to me! I'll see if @rwjblue can make some time to give it a look next week, and I am hopeful we'll put it into FCP at next week's Framework team meeting. Thank you! 👍🏼
One note, which we're going to talk about at next week's "spec" team meeting (a breakout meeting we have with folks from Framework and other teams as makes sense) is whether we will want to add interop with the classic computed property system via notifyPropertyChange()
before actually acting on this RFC, but that would not be a blocker for merging it!
text/0812-tracked-built-ins.md
Outdated
This RFC proposes adding [tracked-built-ins](https://github.com/tracked-tools/tracked-built-ins) | ||
to the blueprints that back `ember new` and `ember addon`. |
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.
Here, we should suggest instead that we want to take the learnings from tracked-built-ins
(and, where possible, the implementation!) and integrate the built-in types directly into @glimmer/tracking
.
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, please ignore this—this was from a long-outdated conversation. That is probably what we will do long-term, but if I recall correctly we wanted to do that after further iteration. I'll make sure we discuss it before putting it into FCP!
Ember had historically shipped `EmberArray` and `EmberObject` | ||
as well as `Map` and `Set` implementations to make it easy | ||
to work with those types in a reactive way. | ||
`tracked-built-ins` does the same for the auto-tracking era. |
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.
`tracked-built-ins` does the same for the auto-tracking era. | |
`tracked-built-ins` demonstrated how to do the same for the auto-tracking era. |
We've moved this to FCP, intending to merge; @wycats is adding some notes on what we need to do to get it to Released and esp. Recommended. |
We need to add a "before-stable" requirement (which doesn't block the approved stage):
|
## How we teach this | ||
|
||
The following guide should be added to the Autotracking In-Depth guide in the official guides, and the | ||
sections on [POJOs][guides-pojos] and [Array][guides-arrays] should be removed. |
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 might be wrong, but the 4.11 docs do not seem to have been updated to reflect the addition of the tracked-built-ins
addon and how to use it like this RFC suggested
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 think at this point, this RFC could/should be moved to the "Released" stage.
Making sure the docs are updated is definitely a requirement for getting it to the "Recommended" stage if I'm not mistaken.
Advance RFC #812 "Add tracked-built-ins dependency" to Stage Ready for Release
Advance RFC #812 `"Add tracked-built-ins"` to Stage Released
Advance RFC #812 `"Add tracked-built-ins"` to Stage Recommended
Rendered