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

More docs #1035

Merged
merged 9 commits into from
Nov 10, 2023
Merged

More docs #1035

merged 9 commits into from
Nov 10, 2023

Conversation

NullVoxPopuli
Copy link
Owner

No description provided.

Copy link

stackblitz bot commented Nov 6, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Nov 6, 2023

⚠️ No Changeset found

Latest commit: 4c1b760

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Nov 6, 2023

Estimated impact to a consuming app, depending on which bundle is imported

js min min + gzip min + brotli
/index.js 26.22 kB 5.71 kB 2.1 kB 1.85 kB
├── core/class-based/index.js 9.39 kB 2.48 kB 1.16 kB 999 B
├── core/function-based/index.js 15.01 kB 4.12 kB 1.55 kB 1.35 kB
└── core/use.js 11.18 kB 3.46 kB 1.4 kB 1.22 kB
/link.js 2.67 kB 376 B 233 B 185 B
/service.js 20.59 kB 5.74 kB 2.12 kB 1.86 kB
/util/debounce.js 3.07 kB 861 B 445 B 373 B
/util/ember-concurrency.js 5.07 kB 1.59 kB 750 B 640 B
/util/fps.js 3.16 kB 919 B 480 B 386 B
/util/function.js 10.06 kB 2.77 kB 1.02 kB 911 B
/util/helper.js 2.12 kB 303 B 218 B 177 B
/util/keep-latest.js 2.08 kB 412 B 261 B 209 B
/util/map.js 5.95 kB 2.44 kB 1.1 kB 924 B
/util/remote-data.js 7.96 kB 2.37 kB 808 B 705 B

Copy link
Contributor

github-actions bot commented Nov 6, 2023

}
```

Note that the _actual_ return value of `resource` is an internal and private object, and interaction with that object is not supported in user-space.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this leads to a lot of confusion. Is here the space to explain why this is the necessary and what tradeoffs are in play?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you expand on this? it's a private object and not meant to be used directly 🤔


This version of `use` does not require decorators, which may be useful if you're in an environment where either you don't have the ability to use decorators, or you are composing contexts, or you want to pass a lazily evaluated pre-configured resource around your application.

The returned value here is a `ReadonlyCell`, meaning you get the value of the resource via a `.current` property.
Copy link

@mattmcmanus mattmcmanus Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you're looking for Docs feedback here....but I'd just thought I'd comment on the confusing DX aspect of this. It's really confusing/hard to keep track of the various things resources return without a lot of trial and error (IMHO). Is there any way to unify what's returned so that there is some consistency?

Copy link
Owner Author

@NullVoxPopuli NullVoxPopuli Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the feedback!! given the following, is this confusing? I don't know if I'm just too deep in the weeds, or if when we were pairing on your first usage of resources, the problem space was just too complicated for a first go-around.

There are only two types of return values:

  • a value
  • a function (aka getter (getters and functions are pretty much the same))
    • in Starbeam's documentation this is a whole api they use, Formula which behaves the same as a @cached getter

The value -- we'll call this Form 1

const Foo = resource(() => {
  return 'theValue';
});

<template>
  {{Foo}} prints 'theValue';
</template>

a function, which is a separate auto-tracking context, like a getter. This is a shorthand for the example after this one for a single value -- we'll call this Form 2

const Foo = resource(() => {
  return () => {
    return 'theValue';
  }
});

<template>
  {{Foo}} prints 'theValue';
</template>

Everything else is 100% up to the user for what APIs the user wants to provide.
For example, here, we'll use an object with getters -- which are lazily evaluated. This is technically Form 1, but combines the concepts of Form 1 and Form 2 by building on the fundamentals of reactivity in javascript (which are presently documented nowhere 😢 I'm going to be working on this a lot very soon):

const Foo = resource(() => {
  return {
    get myOwnThing() {
      return 'theValue';
    }
  }
});

<template>
  {{#let (Foo) as |theValue|}}
    {{theValue.myOwnThing}} prints 'theValue'
  {{/let}}
</template>

This returned value from the resource can be anything, and TypeScript correctly propagates the types, and Glint should be helping folks out when typing this -- if not, we may need to fix Glint.

Using this example tho, we have no need for Form 2, because getters are already lazily evaluated upon access.

We could still define it though, but behavior would be the exact same.

const Foo = resource(() => {
  return () => ({
    get myOwnThing() {
      return 'theValue';
    }
  });
});

<template>
  {{#let (Foo) as |theValue|}}
    {{theValue.myOwnThing}} prints 'theValue'
  {{/let}}
</template>

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe all of this should go in another .md titled "what to return from your resource" or something like that

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to reply to my other comment with one, because they overlap.

I'm trying to sum it up, and what seems odd to me is that how you interact with a resource is so contextually dependent.

  • When you use @use and are in a template you can reference the resource directly
  • When you use use() or compose a decorator with { use } you need to use .current
  • When used in plain JS you access .value off the resource
  • Combining the last 2 means you need to use .current.value

Keep that straight has been a challenge. I know I probably need to broken of this, but concurrency tasks always have a predictable API no matter where it was invoked made them seem so much simpler.

Copy link
Owner Author

@NullVoxPopuli NullVoxPopuli Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sympathetic to having "one way" to do things, but I think it's important to keep the type of thing you're working with in mind, and know what API you're using in any particular situation.

I'm happy add more clarity, to these docs, if there is a place that needs to explain each of the APIs differently -- but it seems like you almost understand everything!

When you use @use and are in a template you can reference the resource directly

and in JS, accessing directly works

When used in plain JS you access .value off the resource

this isn't true, there is no .value

but concurrency tasks always have a predictable API no matter where it was invoked made them seem so much simpler

they have a build time transform, and are always an "object with properties" -- they have a single return value, a TaskInstance -- which provides a lot of utility beyond a single value (concurrency tasks aren't a single value, they are complex objects, lots of properties, lots of methods)

We can't have that here, because you then wouldn't be able to ergonomically use single-values as a resource.

For example:

const Two = resource(() => 2);

class Demo {
  @use two = Two;
  
  <template>
    {{this.two}} 
    => you'd expect this to be '2', and not have to access this.two.value.
       .value would be the *only* property, so why would we bother with that?
       when using a decorator, we don't need to access properties, because 
       decorators allow for lazy *direct* access to values.
  </template>
}

```hbs
{{#let (localizedClock 'en-US') as |innerFunction|}}

{{ (innerFunction) }} -- prints theValue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note about how we are invoking the function inside the curlies, since it's not quite clear why this is needed initially (I didn't quite follow the necessity of the parens inside until the next example).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! However, why wasn't it clear? 🤔 I usually consider the spaces between {{ and ( to add clarity better than the prettier defaults.

it's just the syntax of the template is kind of out of scope for these docs, and is covered by:

https://tutorial.glimdown.com/1-introduction/3-transforming-data

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just initially wondering why the parens were needed, and assumed that innerFunction doesn't execute when used as {{innerFunction}}.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, and we want the function to be invoked so theValue prints

import Component from '@glimmer/component';
import { resource, use } from 'ember-resources';

function LocalizedClock(locale) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LocalizedClock takes the argument of locale, but then never use it. I know it's not the goal of the exercise, but seems like disconnect. What about:

return locale.t(2);

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would apply more implicit information about the example that's not present, like, what defines the function t?

later on do I have /* format defined here */ and then format(time), maybe that type of thing should be in all these examples

Copy link
Contributor

@MichalBryxi MichalBryxi Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, do agree that having to explain extra stuff feels off. Two other possible takes on this:

return new Intl.DateTimeFormat(locale).format(2); // This uses native es6 API

or

function LocalizedClock(localisationFunction) {
  return localisationFunction(2);
}

Can't find what does format(time) refer to, but if the example then is "complete", that'd be IMO better.

No strong feelings though. Maybe it's just me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants