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

invokeHelper argument-based thunks #762

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Aug 11, 2021

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review August 12, 2021 13:01
@ef4
Copy link
Contributor

ef4 commented Aug 20, 2021

I'm not fully convinced this solves the problem better than what can already be done with existing primitives.

It's the author of the helper who knows whether the entangling of argument tracking matters or not. But it's the caller of the helper who would be obligated to pick the right invocation pattern. So this doesn't make it possible for helper authors to solve their own problem reliably.

Instead, a helper author who cares about fine-grained autotracking of several different values could design their API to make that mandatory by making thunks or tracked properties explicitly part of what you're expected to pass in.

For example, nothing stops the author from accepting an argument that it itself a thunk:

invokeHelper(this, SomeHelper, () => ({
  firstArg: () => theTrueValue,
}))

Or accepting an argument with tracked properties:

class HelperArgs {
  @tracked someArgument;
  @tracked someOtherArgument;
}

// ---

let args = new HelperArgs();
invokeHelper(this, TheHelper, () => args);
args.someArgument = 1; // fine-grained invalidation visible to the helper.

@NullVoxPopuli
Copy link
Contributor Author

How would something like this be possible if we don't have argument-based thunks? #434 (comment)

// wrapper around curry because glimmer components only take named args
function component(Klass, thunk) {
  return curry(Klass, () => {
    let named = thunk(); // all args are consumed, regardless of usage
    return { named };
  });
}

let curried = component(MyComponent, () => ({ fruit: this.pepper }))

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Sep 7, 2021

unless... we make a public API for the Lazy Args Proxy class that I've seen duplicated in a couple places 🤔

Like,

let args = argsProxyFor({
  positional: [
    () => this.a,
    () => this.b,
  ],
  named: {
    c: () => this.c,
    d: () => this.d,
  }
});

args.named.c // consumes and evaluates the this.c
args.positional[0] // consumes and evaluates this.a

It'd be cool if we had the same sort of API for all of components/helpers/modifiers

maybe curry could be that api, inspired by invokeHelper?

// example of something I want for components / modifiers / helpers / etc.
// (not a goal of this RFC, but something I do want to work towards)
function component(Klass, namedArgumentThunks) {
  return curry(Klass, argsProxyFor({ named: namedArgumentThunks }));
}

// usage
curriedComponent = component(MyComponent, {
  c: () => this.c,
  d: () => this.d,
};

This is still verbose, but would allow curried components to have lazily consumed args.

idk. Anywho,

I still think we need a way to build out these lazy-evaluating argument objects. I'm open to suggestions, but I don't think the framework provides the building blocks we need atm.

@ef4
Copy link
Contributor

ef4 commented Oct 5, 2021

What I was saying above is that the author of a helper can design their arguments to be independently lazy so their consumption can be tracked independently.

That is different from being able to invoke an arbitrary existing helper in a way that provides the laziness.

And I think that's OK because it's the author of the helper who even knows whether some arguments will remain unconsumed in some important cases. If they know it's important, they can design the arguments to accommodate it.

I still think we need a way to build out these lazy-evaluating argument objects.

I'm pretty sure it doesn't require anything from the framework, just native Proxy.

Finally, an alternative to currying-in-JS would be to implement argument splatting plus GJS or equivalent, which would give a very clear one-liner way to do currying:

import MyComponent from './whatever';
let curriedValue = "Some Title";
let curriedComponent = <template><MyComponent @title=curriedValue ...attributes ...arguments /></template>

And this case avoids all the complexities of visible thunks, which is in one sense the whole part of our declarative template language.

@NullVoxPopuli
Copy link
Contributor Author

thanks!
I'm going to explore both Proxy and getter -using thunks for resources.

argument splatting plus GJS

this is very exciting, and if we added block splatting, that would solve a ton of design problems I'm currently having creating components.

let curriedComponent = <template>
  <MyComponent @title=curriedValue ...attributes ...arguments ...blocks />
</template>

@rwjblue
Copy link
Member

rwjblue commented Oct 8, 2021

After some discussion with the core team today, I am moving this RFC into "FCP to Close".

Based on some conversations that @NullVoxPopuli and I had in discord, using getters for the named properties should allow the laziness desired here without changes to the system (and "its just JavaScript" 🎉 ).

export default class PlusOne extends Component {
  plusOne = invokeHelper(this, RemoteData, () => {
    return {
      positional: [],
      named: {
        get foo() { return this.args.qux; }
      }
    }
  });
}

@rwjblue rwjblue added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Oct 8, 2021
@NullVoxPopuli
Copy link
Contributor Author

a sort of caveat I learned while doing the getter approach with modifiers, and maybe this is obvious... I mean, it makes sense, but this will get you an infinite loop:

class MyComponent extends Component {
  @tracked foo;
  
  myModifier = configureModifier({
    get foo() {
     // `this` is the context of the getter (the object passed to configureModifier), so accessing `foo` here leads to an infinite loop
      return this.foo;
    }
  })
}

so, looking at @rwjblue's example, I tried:

class MyComponent extends Component {
  @tracked foo;
  
  myModifier = configureModifier(() => ({
      get foo() {
        // `this` is *still* the object being returned
        return this.foo;
      }
   }))
}

and lastly:

class MyComponent extends Component {
  @tracked foo;
  
  myModifier = configureModifier(() => {
    return {
      get foo() {
        // `this` is *still* the object being returned
        return this.foo;
      }
     };
   })
}

So, the recommended approach to achieve lazy tracked consumption doesn't work 🤔

Here is a reproduction: NullVoxPopuli/ember-resources#248

@NullVoxPopuli
Copy link
Contributor Author

This "works",

      plusOne = invokeHelper(this, RemoteData, () => {
        return {
          positional: [],
          named: {
            foo: () => this.startingNumber,
          },
        };
      });

but it means that all accesses to foo must now be foo()

@rwjblue
Copy link
Member

rwjblue commented Oct 20, 2021

Ya, sorry about that. I think the syntax would be:

export default class PlusOne extends Component {
  plusOne = invokeHelper(this, RemoteData, () => {
    return {
      positional: [],
      named: {
        get foo: () => this.args.qux,
      }
    }
  });
}

@NullVoxPopuli
Copy link
Contributor Author

that's a syntax error 🤔
image

Build Error (broccoli-persistent-filter:Babel > [Babel: ember-resources]) in dummy/tests/unit/rfc-762-test.js

me/ember-resources/dummy/tests/unit/rfc-762-test.js: Unexpected token, expected "(" (31:19)

  29 |           positional: [],
  30 |           named: {
> 31 |             get foo: () => {
     |                    ^
  32 |               assert.ok(this instanceof PlusOne, `this instanceof PlusOne`);
  33 |               return this.startingNumber;
  34 |             },


Stack Trace and Error Report: /tmp/error.dump.12b0496d20ff2b04f930feb643a321f7.log

@rwjblue
Copy link
Member

rwjblue commented Oct 22, 2021

Eeck. That's what I get for quickly typing out a reply on mobile. The point I was making is still valid, just need to fix the syntax.

Another shot at the syntax is here:

export default class PlusOne extends Component {
  plusOne = invokeHelper(this, RemoteData, () => {
    let componentContext = this;

    return {
      positional: [],
      named: {
        get foo() { return componentContext.args.qux; }
      }
    }
  });
}

But you could also do a nice bit of composition with a class in that location (which would be cool if you were doing more complicated things, and you wanted to unit test just this functionality):

class PlusOneData {
  @tracked args;

  constructor(args) {
    this.args = args;

    this.positional = [];
  }

  get foo() {
    return this.args.qux;
  }
}

export default class PlusOne extends Component {
  plusOne = invokeHelper(this, RemoteData, () => {
    return PlusOneData(this.args);
  });
}

And if that data bag had its own destructor logic (you could imagine it caring about cleanup stuff, depending on what it does), you could do:

import { registerDestructor, associateDestroyableChild } from '@ember/destroyable';

class PlusOneData {
  @tracked args;

  constructor(args) {
    this.args = args;

    this.positional = [];

    registerDestructor(this, () => {
      // do whatever cleanup you need
    });
  }

  get foo() {
    return this.args.qux;
  }
}

export default class PlusOne extends Component {
  plusOne = invokeHelper(this, RemoteData, () => {
    let data = PlusOneData(this.args);
    associateDestroyableChild(this, data);
    return data;
  });
}

@NullVoxPopuli
Copy link
Contributor Author

That makes sense. I guess I was just too tired when I was trying this out to think about assigning this to a variable 🤦

I'll have to play around with these last couple examples (thanks for taking the time to write them! <3 ) and see if I can make a nice public API out of these.

I'm good with closing this RFC now :D

@rwjblue rwjblue closed this Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants