rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Add `tracked-built-ins` dependency

Open SergeAstapov opened this issue 2 years ago • 23 comments

Rendered

SergeAstapov avatar Mar 29 '22 20:03 SergeAstapov

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.

chriskrycho avatar Apr 01 '22 19:04 chriskrycho

Thanks for writing this RFC. I ever wanted tracked built-ins from the get-go of @tracked and was one of the first to suffer from not knowning this.array = this.array is needed.

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 @tracked

The benchmark is the mental model @wycats gave in his keynote this year:

Mark your mutable state as "tracked"

That's enough to explain the mental model of it and simultaneously have a manual for what to do. Now let's see where and how it breaks - a story in three acts:

1. Act: @glimmer/tracking

Especially people new to ember/trying ember the first time, they get the grip of this model real quick and are executing it right away. A code might be looking like this:

import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracked';

export default class MyComponent extends Component {
  @tracked items = [];

  @action
  addItem(item) {
    this.items.push(item);
  }
}

The developer rightfully marked the mutable state with @tracked and was very disappointed to not see any reactivity in their app. Which resulted in blaming ember for announcing its fabulous reactivity system, which then isn't working ?!? Is the developer to blame for not reading into deep why it is not working? No absolutely not - the API is not fullfilling the given promise of marking mutable state with @tracked.

2. Act: tracked-built-ins

Let's continue. Eventually that developer found out to use tracked-built-ins - so fine, change the import to this:

import Component from '@glimmer/component';
import { tracked } from 'tracked-built-ins';

export default class MyComponent extends Component {
  @tracked items = [];

  @action
  addItem(item) {
    this.items.push(item);
  }
}

Alright, done but still not working? Although the mutable state was masked with @tracked? Well under the hood, the decorator is a proxy to @glimmer/tracking only usable for primitive re-asssignments. Instead you have to read through the docs to stumble upon this fact and actually learn, to use items = tracked([]);. With that the API has changed from using @tracked to mask your mutable state to use tracked() to mask your mutable state. Actually, this line @tracked items = tracked([]); has its purpose (thought I wouldn't recommend) - but explains this very-careful-to-use API. Is the developer to blame for not reading into deep why it is not working? No absolutely not - the API is not fullfilling the given promise of marking mutable state with @tracked.

3. Act: ember-deep-tracked

Some might be finding ember-deep-tracked. Apparently, this package fullfills the promise of "mark your mutable state as tracked". You'll go by @tracked something = anything; and it make your application reactive when the tracked state changes.

Considerations for an API

This is growing/becoming one of the most complex and careful APIs to use putting stress on the consumers of this API making their time developing apps less enjoyable.

When it comes to invocation, we've spotted @tracked and tracked() in the wild. I don't mind which of these invocations to use, but I want a consistent usage of them and more importantly not a mixture. I may see a different usage between application and tests. For historical reasons and without any further investigation, this seems fine to me:

  • Application: @tracked
  • Tests: tracked()

Additional "benchmarks" that I consider confusing and a hassle to work with such an API:

  • If I need to think from where I import tracked (!= "don't make me think") => fail
  • If I already have a tracked in my current file, but this is not the correct one and have to import a second import and reassign it to a different name => fail
  • If I read @tracked somewhere I cannot be sure what it actually tracks before reading the import => fail

Ideally, there is only one public export by the whole library: import { tracked } from '@glimmer/tracking'; and that is all consumers ever need.

Summary

As said in the beginning, I like the intent of this RFC to track more things. The way the API is emerging into drives more people to blame ember for its complicated API and turn around to try another framework instead. I would heavily, heavily advise to revisit this API surface.

Personally, I'd like to see a merge of tracked-built-ins and ember-deep-tracked into @glimmer/tracking to make them work under the hood for what they become excellent at but keep them exporting one tracked.

gossi avatar Apr 27 '22 09:04 gossi

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. Though, maybe deep requires a flag to be set, cause perf

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 @glimmer/tracking

NullVoxPopuli avatar Apr 27 '22 11:04 NullVoxPopuli

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 Map, you tracked(theMap) or call new TrackedMap(); doing @tracked someProp = theMap; just does have different semantics (and that’s still true of things with the Stage 3 decorators proposal where that would be @tracked accessor someProp = theMap, it's just more obvious there). While we could make it work there, doing so would be making tracked much, much more complicated a thing to reason about, and while that can seem nice at first it also comes with costs, including to teaching.

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.

chriskrycho avatar Apr 27 '22 18:04 chriskrycho

one of the goals of auto-tracking as a reactivity system is to be as small a layer over normal JS as possible

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).

much, much more complicated a thing to reason about

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:

  • @tracked - reference
  • @tracked({ shallow: true }) - objects/array/map/set // throw on anything else? idk

that deep-tracking should be a built-in capability or especially a default,

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.

NullVoxPopuli avatar Apr 27 '22 19:04 NullVoxPopuli

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 @tracked obj = {} would just work. If tracking deeply by default is a significant performance hit then I'd like to see some type of warning rise up in linting requiring either @tracked(deep: false) or @tracked(deep: true) whenever some non-scalar is tracked. If only certain types of deep tracking are a significant hit then it would be much better for this to work by default and throw a development assertion whenever something too deep is encountered.

jrjohnson avatar Apr 27 '22 19:04 jrjohnson

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 this.array = this.array. I found it exceedingly awkward to import another tracked and then have to name that tracked differently than the original tracked I imported from @glimmer/tracking.

I would really love to see @tracked({deep: true}) as a default, and then the opportunity to do shallow tracking if performance requires it. I think this would help developers learn the api in the correct order and eliminate these road bumps until they are more familiar with the framework or need to optimize.

JimSchofield avatar Apr 27 '22 20:04 JimSchofield

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 new TrackedMap() but I don't think I would often find a need for such a thing (I haven't yet).

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 @tracked. It would be nice to have some low level APIs we can use to make our own performance optimized data objects.

sukima avatar Apr 27 '22 20:04 sukima

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)

NullVoxPopuli avatar Apr 27 '22 21:04 NullVoxPopuli

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:

image

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.

nightire avatar Apr 27 '22 23:04 nightire

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. 💯

👋 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 useState, useEffect with dependency array, useRef etc out of the box to get what you need. No need to google some other packages that might be available and if they are, that they are still maintained as well.

Personally, I'd like to see a merge of tracked-built-ins and ember-deep-tracked into @glimmer/tracking to make them work under the hood for what they become excellent at but keep them exporting one tracked.

☝️ Yes please 🙏

From @NullVoxPopuli

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.

☝️ 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 = [{  }]

☝️ 🤤

devchris avatar Apr 28 '22 09:04 devchris

Notably useState does not have the equivalent of deep tracking, and does have (or is otherwise in the space of implementing) equivalents to useRef and useEffect. I think it‘s one thing to say that Ember should provide comparable tools there—we all agree!—but quite another to determine exactly the right shape of the API for them. @tracked and useState have exactly the same basic behavior—and when you’re updating reference types, you’d more likely reach for useReducer instead, but both useState and useReducer more or less require you to do the functional-style/immutable update! Simply by providing (a) the tracked versions of the most common built-in data structures and (b) the lower-level primitives on which @tracked, TrackedArray, etc. are all built, Ember has provided the same degree of support there.

That does not by itself determine whether we should also add further capabilities in the form of a deep-tracked variant of the tracked decorator, of course. But it’s important to be clear about the actual details of the claims and comparisons we make!

chriskrycho avatar May 06 '22 16:05 chriskrycho

Ember has provided the same degree of support there.

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 :shrug: 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 avatar May 06 '22 16:05 NullVoxPopuli

@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 tracked, much less whether or not it should be the default; it’s simply noting that “We encourage mutation so we should have built-in support for deep tracking in the primary tool used for tracking” is a non sequitur in the strict sense! It’s appropriate and important to consider what patterns our API designs encourage.

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 tracked tool) could be a really useful signal: This is not the default case. That could also be handy for documentation purposes, for simplicity of implementation, for performance (a future where if you don’t use deepTracked, you don’t pay for it), for types (not needing lots of different overloads; overloads in TS are huge pain), etc.

chriskrycho avatar May 06 '22 16:05 chriskrycho

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!

chriskrycho avatar May 10 '22 14:05 chriskrycho

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!

chriskrycho avatar May 10 '22 22:05 chriskrycho

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.) new TrackedObject() can never actually produce the desired type, we don't have the ability to forbid access to the passed-in object, etc.[^rust] There's more design work to be done here, but I'm hopeful we'll have a conclusion there in the next few weeks.

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 tracked() itself, and (c) probably something which can be tackled separately.

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 tracked() can accept a POJO and decorate it, or tracked() can accept configuration options and return a property decorator. It can't do both! (Yes, you could special-case shallow or deep but… that would be terrrrrrrible API design. We’re definitely not doing that!) Net, while deeply-autotracking is an interesting use case, it definitely will not be part of the API of tracked() itself.

[^rust]: Rust-style move/ownership semantics sure would be nice here… 😏

chriskrycho avatar May 17 '22 18:05 chriskrycho

Small comment to cross reference with the ember-modifier RFC, as I think the same constraints apply here and that this RFC will have to do whatever we do there.

https://github.com/emberjs/rfcs/pull/811#issuecomment-1129186872

rwjblue avatar May 24 '22 18:05 rwjblue

@SergeAstapov can you similarly update this one? The updates needed (as far as I recall) are:

  • to match the changes suggested in #811

  • to be explicit that we will be decorating only the 6 types most commonly used in real-world JS today (Array, Object, (Weak)?(Set|Map), but that we are open to adding other built-in data structures in the future if and as usage demonstrates the necessity thereof

  • to specify a design for the constructors, including the relationship to the original object passed in—i.e. that we do a shallow clone on the object, because an API where you mutate the original has extremely surprising results

  • to explicitly state that we are not adding any deep tracking in this case, because:

    • the correct semantics for a framework-level hook there aren't obvious
    • it is possible to build variants on it fairly cheaply in user-land given tracked() as updated here

(Feel free to ping me on Discord if you want/need input on the details there!)

chriskrycho avatar Jun 21 '22 14:06 chriskrycho

I want to add another perspective on the question of tracked(new Map()) vs new TrackedMap(): Extendability.

The target for tracked-built-ins and this RFC is adding tracked version of Array, Object, Map, Set, WeakMap and WeakSet. I think that will cover many use cases. But it would be great to not block adding tracked versions of more built-ins classes in user-land.

The primitives available today should allow building tracked versions of Date, BigInt64Array, BigUInt64Array, Float32Array Float64Array, Int8Array, Int16Array, Int32Array, Uint8Array, Uint16Array, Uint32Array and I guess many more, which I forget about. It is very likely that some projects may want to use tracked versions of them as well.

Extending tracked function provided by @glimmer/tracking would not be possible in user-land. Or at least would require monkey-patching, which we do not want. An addon providing a tracked version of Date would either require exporting its own tracked function, which adds support for Date objects. Or it would need to export a TrackedDate. Let's have a look how that looks in practice:

import { tracked } from 'tracked-date';

const aDate = tracked(new Date());
const aPojo = tracked({});

This pattern might look acceptable at first look. It is actually what tracked-built-ins does today. But it does not scale. As soon as there is a tracked-arrays addon, which provides tracked versions of Int8Array etc. it break. The tracked function from tracked-arrays would not support Date and the tracked function from tracked-date would not support Int8Array. :exploding_head: Consumers would end up doing something like this:

import { tracked } from '@glimmer/tracking';
import { tracked as trackedDate } from 'tracked-date';
import { tracked as trackedArray } from 'tracked-arrays';

const aInt8Array = trackedArray(new Int8Array());
const aDate = trackedDate(new Date());
const aPojo = tracked({});

Let's compare with exporting tracked classes instead of a tracked wrapper function:

import { TrackedObject } from '@glimmer/tracking';
import { TrackedDate } from 'tracked-date';
import { TrackedInt8Array } from 'tracked-arrays';

const aInt8Array = new TrackedInt8Array();
const aDate = new TrackedDate();
const aPojo = new TrackedObject();

The last version seems much cleaner to me.

One might argue, that tracked wrapper function could support all mutable standard built-in objects. But that comes with trade-off of shipping code to many applications, which do not need it at all. Not every application deals with dates. And even less applications use Int8Array and alike.

But there is another even more relevant trade-off: Developers may want to build tracked versions of objects, which are provided by third-party libraries as well. E.g. a team may want to have a tracked version of Moment. We can not know upfront, for which objects a team may want to build a tracked version. We should design an API, which provides a good developer experience if using tracked versions of objects provided by user-land implementations in parallel.

jelhan avatar Jun 24 '22 13:06 jelhan

I want to add another perspective on the question of tracked(new Map()) vs new TrackedMap(): Extendability.

I think this is an excellent point.

The most obvious solution, and the one I think I would favor, is to allow a constructor to register itself as a tracked builtin.

import { registerBuiltin } from "@glimmer/tracking";

class TrackedDate implements Date {
  // ...
}

registerBuiltin(Date, () => new TrackedDate());

const date = tracked(Date);

This is helpful because it makes it obvious both to the user and to the the implementor that what they're doing is constructing something that has the same interface as Date.

That said, I can see that this pattern might have implications for tree shaking that might make it worth considering an alternative. I'm very open to discussion on this.

When thinking through this, I think it's important to remember that the entire space of tracked builtins is limited to the list of JavaScript builtins, and unless an implementation got very elaborate, we'd probably eventually want to pull in any well-implemented version of something like TrackedDate.

wycats avatar Jun 24 '22 16:06 wycats

When thinking through this, I think it's important to remember that the entire space of tracked builtins is limited to the list of JavaScript builtins

Why that? I think it is foreseeable that we want tracked versions of some objects provided by third-party libraries.

Let's stay with the Moment.js example I have used before. Do you see better solved by importing from a wrapper library anyways? Something like:

import moment from 'tracked-moment';

const date = moment();

jelhan avatar Jun 24 '22 19:06 jelhan

@jelhan I take your point.

My rough intuition, which I'm very open to changing, is that it makes sense to have a single entry point for the tracked builtins that map directly onto stuff in the ECMA-262 spec. This is both to make it super-clear that the intent is for the APIs to be 1:1 with the spec, as well as to establish core team ownership over the stuff that has a clear spec for quality control reasons.

Your pushback, aiui, is:

  1. There are going to be third-party implementations of ECMA-262 features (like tracked typed array) for the forseeable future.
  2. Other stuff, like moment, isn't so different in the mind of an average developer.

And therefore: having a handful of tracked builtins go through a single entry point, while seemingly similar APIs come through addons is a recipe for confusion.

Is that about right?

If so, what do you think is the best way to communicate the fact that the core team owns a large subset of the 262 tracked builtins, and that it is considered a fairly urgent bug if there is a difference in behavior from the spec?

As an aside, we might want to try to run some subset of the ECMAScript tests (specifically, the ones that deal with instance behavior). This wouldn't necessarily communicate the point to a broad audience, but I bet it would catch bugs and help us avoid introducing bugs!

wycats avatar Jul 06 '22 18:07 wycats

sorry for the delay here

Small comment to cross reference with the ember-modifier RFC, as I think the same constraints apply here and that this RFC will have to do whatever we do there.

#811 (comment)

added the requested tweak in commit https://github.com/emberjs/rfcs/pull/812/commits/ca8525d8046c36f599274f2f18f819f08294c404

* to be explicit that we will be decorating only the 6 types most commonly used in real-world JS today (`Array`, `Object`, `(Weak)?(Set|Map)`, but that we are open to adding other built-in data structures in the future _if and as usage demonstrates the necessity thereof_

* to specify _a_ design for the constructors, including the relationship to the original object passed in—i.e. that we do a shallow clone on the object, because an API where you mutate the original has _extremely_ surprising results

* to explicitly state that we are _not_ adding any deep tracking in this case, because:
  
  * the correct semantics for a framework-level hook there aren't obvious
  * it is possible to build variants on it fairly cheaply in user-land given `tracked()` as updated here

added the requested tweaks in commit https://github.com/emberjs/rfcs/pull/812/commits/401a58bfc052aa5901c3f731c51c33663cd2e6be

@chriskrycho @rwjblue please let me know if this does not match exactly what you were thinking/proposing.

SergeAstapov avatar Sep 22 '22 16:09 SergeAstapov

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.

chriskrycho avatar Dec 02 '22 19:12 chriskrycho

We need to add a "before-stable" requirement (which doesn't block the approved stage):

  • before advancing this change to "ready to release", @glimmer/tracking and @glimmer/validator need to be implemented as normal JS packages instead of being hardcoded in the ember-source build.

wycats avatar Dec 02 '22 19:12 wycats