Skip to content
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

`@use` and Resources #567

Open
wants to merge 5 commits into
base: master
from
Open

`@use` and Resources #567

wants to merge 5 commits into from

Conversation

@pzuraq
Copy link
Contributor

pzuraq commented Dec 23, 2019

Rendered

pzuraq added 2 commits Dec 17, 2019
@alexlafroscia

This comment has been minimized.

Copy link

alexlafroscia commented Dec 23, 2019

Would it be worthwhile to think through what a testing utility might look like for Resources? It might be helpful to have a way to test them outside of creating a dummy component that uses it and then interacting with the faux component.

I’m imagining something sort of similar to what react-hooks-testing-library provides for React Hooks.

import { use } from @ember/test-helpers’;

test(‘reacting to a state change’, function(assert) {
  this.foo = ‘some state’;
  const resource = use(Resource.create(this.foo));

  assert.equal(resource.state, ‘whatever’);
});

I’m imagining that the result of use from the test helpers could include a means to look at the state of the resource and invoke the tear-down logic. It would be great if updating the arguments, through the context of the test itself, could cause the start or update hooks to fire as appropriate, so that there’s symmetry between testing the response to changes in a test and how a Resource would react to a component’s state changing.

One question that comes to mind is whether there needs to be a way to mark the testing properties on the test context as being tracked, so that updating them in the test context behaves in such a way that it can be updated and the Resource reacts to the change, as it does when creating a Resource with component state/args as arguments.

@sclaxton

This comment has been minimized.

Copy link

sclaxton commented Dec 23, 2019

I think the major issue with naming, which was partially touched on in the RFC, is that a "resource" typically refers to nouns, à la REST, yet a primary use case here is to support declarative side-effects, like the Interval example. Maybe it's just me, but it feels pretty difficult for me to fit side-effects into the general mental model I currently have for what "resource" means in the context of most other software...Not that it can't be retaught but definitely a significant amount of cognitive baggage there.

That being said, one alternative to get around the above confusion could be to follow suit with React and separate the side-effect API from the state API? Instead of exposing one base class, expose two separate interfaces called something like ManagedState and ManagedEffect?

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Dec 23, 2019

@sclaxton

...what "resource" means in the context of most other software

Based on my research for this RFC, I actually believe that REST is the exception, not the rule. Most other software uses of the term "resource" actually refer to something that has a managed lifecycle, as in the Resource-Acquisition-Is-Initialization pattern. This is discussed briefly in the RFC in the Name Choice section.

I do agree though that within the context of the web, "resource" has another distinct and not-uncommon meaning. We did consider various combinations of the words Manage and State, including StateManager. ManagedState and ManagedEffect make some sense, if we end up having separate managers for separate uses. I still think Resource makes the most sense though, personally.

@sclaxton

This comment has been minimized.

Copy link

sclaxton commented Dec 24, 2019

Based on my research for this RFC, I actually believe that REST is the exception, not the rule. Most other software uses of the term "resource" actually refer to something that has a managed lifecycle, as in the Resource-Acquisition-Is-Initialization pattern. This is discussed briefly in the RFC in the Name Choice section.

Definitely agree that Resource à la RAII makes sense as the low-level primitive. I just wonder if exposing this at a higher level via more use-case centric interfaces (e.g. something like ManagedState, ManagedEffect) would be helpful for teaching it.

EDIT: Also I guess what I meant by "resource in the context of other software" was more along the lines of it usually mapping to the concept of a "material entity", e.g. a section of memory, a lock/mutex, etc. So the observation I was attempting to make is that a side-effect doesn't seem to map to the same concept of a material entity in my mind.

@jgwhite

This comment has been minimized.

Copy link

jgwhite commented Dec 29, 2019

Could you discuss the motivation for having @use return the resource’s state property (rather than a reference to the resource itself)?

text/0567-use-and-resources.md Show resolved Hide resolved
text/0567-use-and-resources.md Show resolved Hide resolved
text/0567-use-and-resources.md Outdated Show resolved Hide resolved
```

This helper will create and invoke the resource dynamically, with the same
semantics as `@use`. Since helpers are always consumed, resources used with

This comment has been minimized.

Copy link
@simonihmig

simonihmig Dec 29, 2019

Contributor

How would the helper args map to the arguments passed to start()/ update()? I.e. will only positional params work, or also named?

This comment has been minimized.

Copy link
@pzuraq

pzuraq Dec 29, 2019

Author Contributor

Correct, we should state this explicitly, but the goal is to have as close to a 1-1 API as possible, so named arguments wouldn't make sense.

resources to be setup based solely on the template.

```hbs
{{use SyncDocumentTitle @title}}

This comment has been minimized.

Copy link
@simonihmig

simonihmig Dec 29, 2019

Contributor

I guess to be able to work with state in the template, basically every use of the helper would have to look more like this (unless you are only interested in the side effect, as in this example, and not in the returned state):

{{let (use this.counter) as |state|}}
  Count: {{state.count}}
{{/let}}

I think that's fine, to keep the APIs orthogonal, but it tends to get a bit verbose though...

This comment has been minimized.

Copy link
@pzuraq

pzuraq Dec 29, 2019

Author Contributor

So that example would actually be more like:

{{#let (use this.counter) as |count|}}
  Count: {{count}}
{{/let}}

Since the use will return the state property of the resource directly, like the decorated property. Simple cases would probably work pretty well, but more complex cases would definitely get verbose. I think the "let for the rest of the template" idea could help here, potentially.

fundamentally related to the existing state flow patterns that were established.
They should be introduced in the `Components` section of the guides, after
modifiers. The docs should include useful examples for how to accomplish basic
tasks, such as writing a `RemoteData` resource.

This comment has been minimized.

Copy link
@simonihmig

simonihmig Dec 29, 2019

Contributor

As this RFC not only proposes the low-level manager API, but also a ready to use Resource class, what does this imply for Ember CLI?

  • will ember g resource and ember g resource-test be introduced?
  • will there be an "official" new app folder /resources where resource classes live by default?

This comment has been minimized.

Copy link
@pzuraq

pzuraq Dec 29, 2019

Author Contributor

That's a great question. I think generating the resource and resource test would be good, though I'm not sure where they should go.

I don't think we want to add a new top level /resources folder - my aim was to not add resources as a top-level resolvable, and to encourage defining them inline or nearby to the place they are used, more like utilities. There will definitely be some common resources that are used in many places, but there will also likely be some resources that are one-offs, only used in one place in the app.

Maybe they could go under /app/utils/resources? This really starts to get into how generators will work in general in a more import-driven world.

This comment has been minimized.

Copy link
@Gaurav0

Gaurav0 Jan 1, 2020

Contributor

Disagree with putting this in a Components section of the guides. This seems complex enough and useful enough to deserve its own section in the guides. IMO this is a top level construct similar to a React Context.

This comment has been minimized.

Copy link
@Gaurav0

Gaurav0 Jan 1, 2020

Contributor

That's a great question. I think generating the resource and resource test would be good, though I'm not sure where they should go.

I don't think we want to add a new top level /resources folder - my aim was to not add resources as a top-level resolvable, and to encourage defining them inline or nearby to the place they are used, more like utilities. There will definitely be some common resources that are used in many places, but there will also likely be some resources that are one-offs, only used in one place in the app.

Maybe they could go under /app/utils/resources? This really starts to get into how generators will work in general in a more import-driven world.

I agree resources should not be resolvable by the resolver. They should be imported as described here. However, I think for organizational purposes it should be recommended that a top level resources folder be created and used for resources.

IMO this is a top level construct similar to a React Context.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Dec 29, 2019

@jgwhite absolutely, there are a few reasons:

  1. Resources are intrinsically tied to the lifecycle of the class they are defined on. As such, they cannot be moved around, borrowed, or reassigned in the same way other standard properties can. Preventing direct access to the resource itself makes it more difficult for users to do things that wouldn't make sense in this paradigm (e.g. passing the resource as an argument to child components).

  2. Resources, like components, will usually have some amount of internal/private state that they usually would like to keep hidden, along with their public state. At the least, the start, update, and teardown methods would be "friend" APIs that only the framework should call directly. Returning the state property allows resources to expose well-defined public APIs, and keep other implementation details private. (Note: This particular issue is exacerbated by the fact that decorators currently do not work on private class fields, which means that users cannot use those to hide state effectively. Symbols could work potentially, but have their own caveats like browser compatibility).

  3. Exposing the state property means that ultimately, the resource can be treated somewhat like an implementation detail. For instance, you could use a resource to load a collection from a data store, without having to tie the data store directly to Ember - the resource handles taking derived state and transforming it into a collection, which it then exposes. By contrast, if we exposed the resource directly, it would mean that we would either have to make the store implement the collection as a resource, or we would have another layer of indirection for the collection (e.g. {{this.collection.values}}).

Edit: Found another reason - Because we're tearing down and creating new instances of the Resource by default, it prevents users from accidentally passing around stale versions of the resource. I think this is pretty important in the end, since ending up with a stale version could definitely cause issues.

text/0567-use-and-resources.md Outdated Show resolved Hide resolved
@jgwhite

This comment has been minimized.

Copy link

jgwhite commented Dec 31, 2019

@pzuraq thanks for the fantastic explanation. That all makes a lot of sense.

Do you think there’s any risk that Resource.create(...) will cause confusion for folks (like myself) who are very used to Foo.create(...) creating an instance of a classic Ember class? Is there any merit in discussing alternatives to create, if only to hint that a different mechanism is at play?

@use products = RemoteData.for(this.args.searchUrl)
@use products = RemoteData.from(this.args.searchUrl)
@use products = RemoteData.compute(this.args.searchUrl)

On a related note, why not @use products = new RemoteData(this.args.searchUrl)? (I’m guessing this has something to do with the TypeScript workaround mentioned in the RFC)

@tchak

This comment has been minimized.

Copy link
Member

tchak commented Dec 31, 2019

I agree with @jgwhite – I feel that create might be confusing.
I quite like

@use products = RemoteData.for(this.args.searchUrl)
@Gaurav0

This comment has been minimized.

Copy link
Contributor

Gaurav0 commented Jan 1, 2020

ember-concurrency does many things that resources do not do; but I think can complement resources if and when task cancellation is required. We should add a sentence like:

"Rather than replacing ember-concurrency, resources can be used to trigger ember-concurrency tasks while ember-concurrency continues to handle async needs when the ability to cancel a task is required or desired."

I'd love to also see a full example of using ember-concurrency in a resource.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 1, 2020

@Gaurav0 the language discussing replacing/rewriting libraries like ember-concurrency was very intentional, and Resources are designed to address some of the needs that concurrency tasks currently have. Three specific needs jump out:

  1. Decoration. Concurrency tasks were built on the ComputedProperty base class, utilizing private API to allow themselves to decorate and set themselves up on classes. This method for defining tasks is not ideal, and Resources would provide a better foundation for a more modern solution. The alternative would be for ember-concurrency to create its own decorators entirely, which would be possible, but would definitely be limited.

  2. Cancellation. All concurrency tasks currently attempt to cancel themselves if the item they are defined on has been destroyed. They currently patch the willDestroy method of the classes they are defined on, which is a brittle solution as it relies on, generally, an EmberObject based API. The first non-EmberObject framework class, GlimmerComponent, also conforms to this API, but this is not a guaranteed destruction pattern, so a more general solution would be better.

  3. Observation. ember-concurrency tasks do currently have an undocumented .observes() modifier for rerunning when particular properties change. I'm not familiar with the history there, but I would hazard a guess that the reason it's not documented is a combination of observers falling out of favor in general, and the availability of lifecycle hooks for triggering tasks when args change. Now that lifecycle hooks don't exist, we'll need a new pattern, and it makes sense to build it directly in, conventionally.

If the concern is that tasks are also used for action-driven, rather than consumption-driven, purposes, then I do think the Resource API as proposed is flexible enough to handle this. You could imagine a future task API where modifiers are used to signal if a task should run automatically, or if it should wait to be triggered by an action or external event:

class MyComponent extends Component {
  @use loadDataTask = task(function*() {
    // ...
  }).auto();

  @use saveDataTask = task(function*(arg1, arg2) {
    // ...
  });

  @action
  saveData() {
    this.saveDataTask.perform()
  }
}

If there are other specific concerns that you have that make you think Resources would not be a good fit for a primitive to rebuild ember-concurrency on top of, I'm definitely interested, because I think this is a very important use case for us to support with them.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 2, 2020

@jgwhite @tchak agreed that the naming conflict is not ideal. We did consider other names, specifically .from() and .with(), but they're a bit awkward if you have a Resource that doesn't accept arguments:

@use counter = Counter.for();
@use counter = Counter.from();
@use counter = Counter.with();
@use counter = Counter.compute();

I think .compute() works out of these, but still feels a bit off.

On a related note, why not @use products = new RemoteData(this.args.searchUrl)? (I’m guessing this has something to do with the TypeScript workaround mentioned in the RFC)

This actually has two purposes, one being the type work around, but the other is a bit more nuanced and should probably be spelled out more explicitly in the RFC.

The initializer is technically a thunk that we are using to accomplish two tasks:

  1. Get the Resource definition itself
  2. Get the argument values that are passed to the Resource during both start and update.

When we want to update a Resource, we need to be able to rerun this thunk in order to get the new values of those arguments, but without creating a new instance of the Resource. Using static methods, we could do this like so:

class Resource {
  static create(...args) {
    let { isCreating } = getUseInfo();
    
    setUseArgs(...args);

	if (!isCreating) {
      return new this();
    }
  }
}

This way, the first time we run the thunk, it will create the resource and save off the arg values. The second time, it will just save off the arg values.

The dual nature of this thunk is definitely not clear cut, and we've received feedback already from a few folks that it's a bit counterintuitive that a static create method (or any other static method) would conditionally return an instance. On top of that, we've explored typing static methods and found out that they actually can't be typed in the way that we originally were hoping. With that in mind, I'm planning on updating the RFC to a propose a tweaked API:

class RemoteData {
  @tracked data = null;
  @tracked isLoading = false;

  get state() {
    return {
      isLoading: this.isLoading,
      data: this.data,
    };
  }

  async start(url) {
    this.isLoading = true;

    let response = await fetch(url);
    let data = await response.json();

    if (!this.isDestroyed) {
      this.isLoading = false;
      this.data = data;
    }
  }
}

export const remoteData = resource(RemoteData);
// usage
@use data = remoteData(this.args.url);

This API is type-able, and doesn't introduce the same issues with the dual purpose of create (though it does introduce some "magic", with an opaque wrapper/constructor function).

It also would potentially solve an impending problem with class-based helpers/modifiers and template imports, if it were mirrored over to them. Currently, both class-based and functional helpers are normalized to snake-case when used, but with template imports that'll change to use the direct identifier. This means that we'll be invoking class-based helpers/modifiers with capital case:

---
class Translate extends Helper {}
---

{{Translate "foo"}}

If we followed this pattern of wrapping classes, it'd allow us to normalize all the identifiers to camelCase:

---
class Translate {}

const translate = helper(Translate);
---

{{translate "foo"}}

I think that this should be decided in a separate RFC, ultimately, but it's worth considering how our choices for this API could affect the future there.

@jgwhite

This comment has been minimized.

Copy link

jgwhite commented Jan 2, 2020

@pzuraq the resource wrapper function makes a lot of sense to me, and I don’t think the fact that it’s a black box is a problem. To me the mental model seems to be:

  1. @use declares a property as a “resource context” (with a thunk that can comfortably masquerade as an initializer)
  2. resource creates a function that runs a resource in a “resource context”
  3. “resource context” is a little environment managed by the framework with the prescribed start/update/teardown lifecycle
@tchak

This comment has been minimized.

Copy link
Member

tchak commented Jan 4, 2020

Will @use only work in components? What about routes? One cool feature of ember-concurency is that it works also in routes. I am also thinking of use cases in ember-apollo, ember-data or ember-orbit where we want to return a "live" query result (from a route or a component) and bind it to the component/route lifecycle. Maybe it is out of scope of @use.

@jgwhite

This comment has been minimized.

Copy link

jgwhite commented Jan 4, 2020

@tchak I think it’ll work on all framework-managed things. Per the RFC:

The following framework provided classes will use this function by default:

  • EmberComponent
  • GlimmerComponent
  • Helper
  • Route
  • Controller
  • Service
  • Location
  • Resource
@josemarluedke

This comment has been minimized.

Copy link

josemarluedke commented Jan 8, 2020

I love the direction here; this is definitely something missing in the Octane programming model.

  • I have to agree with other comments regarding the name Resource. My main issue with the name it's too broad. For folks that want to search on Google about Resource might have a hard time. A more specific name as ManagedHook or TrackedHook would help a ton for communication, search engines, documentation, and articles.
  • The first thing I thought when I saw .create in the example was, "why do we need EmberOject here; I want just to do new MyClass()... " As I read more, I understood why, but it would be nice to find a new method name for that. I really like the resource wrapper function; it also means that the Resouce implementation (user-defined class, eg. RemoteData) does not need to extend from a framework class.
  • It seems that Resources are going to have access to the owner; therefore, we should be able to inject services into it. Is that correct?
@jessica-jordan jessica-jordan mentioned this pull request Jan 10, 2020
10 of 28 tasks complete
@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 11, 2020

@josemarluedke yes, Resources will have access to the owner and injections, that is definitely crucial to the design. One thing to note though is that with the most recent design change and the addition of the wrapper function, the owner will not be set on the Resource until after construction. It'll be available by the time setup is called, but it will not be available during constructor or class fields.

Personally I think this is probably fine, and the owner will still be a constructor parameter so if you want to manually do setOwner or extend from a base class it's still possible.

// usage
class MyComponent extends Component {
@use counter = counter(this.args.interval);

This comment has been minimized.

Copy link
@scottmessinger

scottmessinger Jan 12, 2020

I'm finding this example a bit confusing because of the use of counter a few different ways. In the example above, you use different words on either side of the equals which I find easier to follow (products = remoteData). So, in a similar way, what if this was something like:

Suggested change
@use counter = counter(this.args.interval);
@use counter = counterResource(this.args.interval);

This would also mean changing the variable on 243 to const counterResource = resource(Counter)


```js
export default class SearchResults extends Component {
@use products = remoteData(this.args.searchUrl);

This comment has been minimized.

Copy link
@scottmessinger

scottmessinger Jan 12, 2020

Going along with my comment below, I think could be even more clear by renaming it to:

Suggested change
@use products = remoteData(this.args.searchUrl);
@use products = remoteDataResource(this.args.searchUrl);
@pzuraq pzuraq force-pushed the use-and-resources branch from 49fef9c to a89b7a1 Jan 12, 2020
type ResourceDefinition = { resource: ManagedResource };
type ResourceDefinitionThunk = () => ResourceDefinition;
interface ResourceInstance {

This comment has been minimized.

Copy link
@rwjblue

rwjblue Jan 12, 2020

Member

ZOMG LETS NOT INCLUDE INSTANCE IN AN INTERFACE / CLASS NAME AGAIN PLZ?!?!?!!?!

😸 ❤️

This comment has been minimized.

Copy link
@chriskrycho

chriskrycho Jan 13, 2020

Contributor

We should come up with a name not as apt to troll you for sure. 😆

I’d suggest we invert this: it should be interface Resource and if we need to name the type of the resource constructor, differentiate that as interface ResourceConstructor. We may need to make that distinction here because of how TS conceptualizes its types, i.e. that you do have to distinguish between the type and it’s constructor. However, as this is designed currently, I don’t think we need that: we can just specify that they type for the argument to resource is a Resource, full stop?

(I’ll have more comments on the types proposed here later this week. @pzuraq and @dfreeman and I have played with a bunch of variants in the design space for the types, some of them better than others; this is part of how we identified the constraints around static methods described in the RFC. However, the types as they are in the proposal here represent a fairly rough draft: I just threw together the quickest thing that could possible work when we started discussing this version of the proposal, to see if it was viable at all. As such, we’d like to polish them a fair bit, and now rather than post-RFC, so that what the Typed Ember crowd ends up doing on DefinitelyTyped can actually match the design here. Otherwise we’re asking for confusion in the community, and nobody wants that! 😂)

constantly in a repetitive manner. The core of the API consists of:

- The `@use` decorator (and the complementary `{{use}}` helper)
- The resource base class (implemented via a `ResourceManager`, similar to

This comment has been minimized.

Copy link
@jgwhite

jgwhite Jan 13, 2020

Am I right in thinking the base class is no longer part of this proposal?

@jgwhite

This comment has been minimized.

Copy link

jgwhite commented Jan 13, 2020

It seems like the proposed timing for @use is now “start when first consumed” (now that scheduleUseStart is gone). Do you think this could cause confusion given that a @use expression looks so much like an initializer?

For example, one might be tempted to refactor the interval example using @use:

  class MyComponent extends Component {
-   constructor() {
-     useResource(this, () => interval(this.sayHello, this.args.interval));
-   }
+   @use interval = interval(this.sayHello, this.args.interval);
  
    sayHello() {
      console.log('Hello!');
    }
  }

At a glance these two declarations look equivalent, but based on the detailed design of @use and this passage from the alternatives section it seems like the @use version would actually be a no-op unless one were to consume this.interval somewhere. Maybe I’m overthinking it, but I can imagine being quite puzzled to find the refactor didn’t have the desired effect.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 13, 2020

@jgwhite that is a consideration that we've been mulling over, and it is true this could be a bit confusing. There were two reasons driving the change:

  1. There was a lot of confusion over resources that did not expose any value at all, such as side-effects, being assigned to a property in the first place. After all, the property would always return undefined, so it seemed strange that it would be necessary. In the current design, the imperative API allows us to distinguish between resources that are meant to have values, and resources that are not.

  2. There was also the consideration that initializing the fields at a different point in time, potentially much later on, could be just as confusing as not immediately initializing them. The timing semantics of class fields are important, and the fact is that we would be changing them from "immediately" to "sometime later", and that could be just as problematic for user's mental models.

One alternative design would be to decorate getter functions instead:

@use
get interval() {
  return interval(this.args.handler, this.args.interval);
}

While this would imply laziness, it introduces another problem in that we really don't want or expect this getter function to be dynamic. For instance, we would want to avoid something like this:

@use
get interval() {
  if (this.args.interval) {
    return interval(this.sayHello, this.args.interval);
  } else if (this.args.timeout) {
    return timeout(this.sayHello, this.args.timeout); 
  }
}

While the same kind of dynamism is definitely possible in a class field, it is much harder to do, and I think this will discourage it effectively. Personally, I would prefer accepting laziness from the decorator if it means we get to keep the class field syntax.

@jrjohnson

This comment has been minimized.

Copy link

jrjohnson commented Jan 13, 2020

@pzuraq i'm not sure how:

@use
get interval() {
  if (this.args.interval) {
    return interval(this.sayHello, this.args.interval);
  } else if (this.args.timeout) {
    return timeout(this.sayHello, this.args.timeout); 
  }
}

is any less likely than

@use interval = task(function*() {
  if (this.args.interval) {
    return yield interval(this.sayHello, this.args.interval);
  } else if (this.args.timeout) {
    return yield timeout(this.sayHello, this.args.timeout); 
  }
});

and I'm not sure why avoiding a dynamic getter function should be avoided. Can you expand on this point? I would have assumed either of these was a valid case.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 13, 2020

@jrjohnson the later is perfectly valid though, because the task itself is an outer resource, and in this case interval or timeout would not actually be resources, they would (presumably) be functions that return promises. The point of the getter/initializer is that it is providing the definition of the resource, not the actual logic it contains.

@jgwhite

This comment has been minimized.

Copy link

jgwhite commented Jan 13, 2020

There was also the consideration that initializing the fields at a different point in time, potentially much later on, could be just as confusing as not immediately initializing them. The timing semantics of class fields are important, and the fact is that we would be changing them from "immediately" to "sometime later", and that could be just as problematic for user's mental models.

Perhaps I’m misunderstanding this point. Wouldn’t a resource begin emitting state immediately? So the field would appear to be initialized from construction, albeit with { isLoading } or something of that manner?

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 13, 2020

@jgwhite the point is that it's not possible to initialize them in the exact same order. Consider:

class Foo {
  bar1 = console.log(1);
  bar2 = console.log(2);
  bar3 = console.log(3);
}

new Foo(); // 1, 2, 3

Now, let's say we make @use with scheduleUseStart as originally proposed, and turned bar2 into a resource.

class Foo {
  bar1 = console.log(1);
  @use bar2 = logResource(2);
  bar3 = console.log(3);

  constructor() {
    // bar1 and bar3 have been initialized by this point
    scheduleUseStart(this);
  }
}

new Foo(); // 1, 3, 2

No matter what we do here, the difference in timing will be observable. We can also observably change these timing semantics by accessing the field, so for instance:

class Foo {
  bar1 = console.log(1);
  @use bar2 = logResource(2);
  bar2Starter = this.bar2;
  bar3 = console.log(3);

  constructor() {
    scheduleUseStart(this);
  }
}

new Foo(); // 1, 2, 3

Which seems odd. Always being consumption based makes this much simpler overall, it means you need to learn that @use changes things, but you only need to learn it once, and it's consistent.

@jgwhite

This comment has been minimized.

Copy link

jgwhite commented Jan 13, 2020

@pzuraq ah, that’s a wonderful illustration, thank you.

So are we saying that users would have to learn that the @use form is really not an initializer and won’t behave according to their expectations for an initializer? I’m not necessarily saying this is insurmountable, but I feel compelled to hash it out 😅

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 13, 2020

@jgwhite correct, I think what I'm saying is no matter what, if we want to replace the field with a getter, it will change some things. We can try to minimize that gap and maybe if it's small enough, we won't need to teach it, but my intuition is that it'll at least need to be in an "advanced" section of some kind for edge cases like these.

One alternative would be for @use to replace the field with another field. It would mean that users would always have to add an extra chain (e.g. this.myResource.state, this.myTask.state.isPending), but would mean we could keep the initializer semantics. It's also possible decorators could change in the future and allow us to initialize a getter (🤞 I'm very much hoping for this).

And no worries, this is the perfect time to hash these things out 😄 this API is admittedly trying to solve a lot of concerns within a very narrow design space.

@jgwhite

This comment has been minimized.

Copy link

jgwhite commented Jan 13, 2020

@pzuraq given that useResource and the core of the mechanism is looking solid, is there an argument to maybe split @use into a separate RFC?

@chriskrycho

This comment has been minimized.

Copy link
Contributor

chriskrycho commented Jan 15, 2020

As a little data point in favor of this design, I was looking at an Ember component in our app today, which is 57 lines of JavaScript (excluding comments and whitespace) and no template and thinking about how I'd refactor it into a Glimmer component, and especially thinking about whether it could be a template-only component. The existence of template-space use and the rest of this design took it from almost template-only friendly, but still needing a backing class for one piece, to completely doable in template space.

And those 57 lines of JavaScript would be reduced to a whopping 9 lines of template code (formatted generously! It's 6 formatted the way Prettier would format it) and 28 lines of JS (including the imports).

Better than the reduction in lines of code, though, this is a dramatic improvement on the separation of concerns and therefore the maintainability and testability of the code. Each piece of it is much more self-contained and only knows what it has to.

@ef4 ef4 mentioned this pull request Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.