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 #626

Merged
merged 9 commits into from
Jun 22, 2020
Merged

invokeHelper #626

merged 9 commits into from
Jun 22, 2020

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented May 8, 2020

@jelhan
Copy link
Contributor

jelhan commented May 10, 2020

Would it be possible to implement invokeHelper in an addon using the proposed Helper Manager?

@pzuraq
Copy link
Contributor Author

pzuraq commented May 10, 2020

It would not. There is no way to lookup a helper manager instance using public APIs, and the hooks on helper managers are meant to be wrapped by the VM directly. For instance, runEffect needs to be scheduled by the VM at the correct point in the lifecycle of the helper.

This is the minimum API we can expose in order to invoke a helper in the same way as it would be if done by the VM, which is the goal.

text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
Comment on lines +282 to +283
Helpers work, but like components they require us to move some of our logic
into the template, even if that isn't really necessary:
Copy link
Member

@rwjblue rwjblue Jun 5, 2020

Choose a reason for hiding this comment

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

I'm not sure I follow this part. What is the unnecessary part in the template? Just the invocation of the helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, exactly. You can't refer to the value of the helper in JS anymore, so any computed properties you have that may rely on the value of the helper now need to be in the template, any other business logic now needs to be moved into the template, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't see that as unnecessary 🤔

Either way (either with this RFC or with templates) you have to invoke the helper. The only question is where that invocation actually is.

text/0626-invoke-helper.md Outdated Show resolved Hide resolved
Comment on lines +341 to +342
reactive code and patterns, and reuse them transparently. This will make helpers
the new reactive atom of the system - the reactive equivalent of a "function" in
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I don't think this actually changes the atom, it just makes helpers an equal peer to components. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Helpers are already a peer. The issues is neither helpers nor components can be used in JS-only. This API makes helpers better than components for many use cases, and makes them usable in many more places, so I would say they are the reactive atom now. Especially considering Glimmer components have no lifecycle hooks, so they can't even do the same things anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but the prose here infers that we should now always decompose components into helpers because "they are the smallest atom", which I don't think is correct either.

text/0626-invoke-helper.md Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
text/0626-invoke-helper.md Outdated Show resolved Hide resolved
@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2020

We discussed this in today’s core team meeting, and think it is ready to move into final comment period.

🥖🥳🕺

@betocantu93
Copy link
Contributor

Great feature, I definitely would use this. Just one quick question, not sure if I'm missing something but in your example

import Component from '@glimmer/component';
import Helper from '@ember/component/helper';
import { invokeHelper } from '@ember/helper';

class PlusOne extends Helper {
  compute([num]) {
    return number + 1;
  }
}

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

You meant RemoteData to be PlusOne (the helper)? something like

import Component from '@glimmer/component';
import Helper from '@ember/component/helper';
import { invokeHelper } from '@ember/helper';

class PlusOneHelper extends Helper {
  compute([num]) {
    return number + 1;
  }
}

export default class PlusOne extends Component {
  plusOne = invokeHelper(this, PlusOneHelper, () => {
    return {
      positional: [this.args.number],
    };
  });
}

@scottmessinger
Copy link
Contributor

I'm excited for this to land -- definitely will be useful for us.

The part I'm confused by is the computeArgs return value. If the positional value is always an array and named is always an object, why not just return either an array or an object from the function? Being familiar with helpers, the idea of calling it positional makes sense, but if I didn't know helpers can have positional arguments, I think I'd be at a loss to explain this API.

Not knowing the limitations that might have led to this design choice, I would prefer if the types look more like this:

-interface TemplateArgs {
-  positional?: unknown[],
-  named?: Record<string, unknown>
-}
+type TemplateArgs = unknown[] | Record<string, unknown>

type HelperDefinition<T = unknown> = object;

function invokeHelper<T = unknown>(
  parentDestroyable: object,
  definition: HelperDefinition<T>,
  computeArgs?: (context: object) => TemplateArgs
): Cache<T>;

@jelhan
Copy link
Contributor

jelhan commented Jun 17, 2020

If the positional value is always an array and named is always an object, why not just return either an array or an object from the function?

A helper could use both positional and named arguments at the same time. E.g. {{format-date date timeZone="UTC"}} from ember-intl.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jun 19, 2020

@scottmessinger @jelhan's explanation is correct, it is not an either-or API, both can be passed in at the same time. This API is also meant to be a primitive API - the idea is that higher level APIs will be written on top of this to provide easier to use, more ergonomic DX. Those APIs could add the logic you're describing for instance.

@rwjblue
Copy link
Member

rwjblue commented Jun 22, 2020

We discussed this in Friday's core team meeting, and think it is ready to land. Thank you to @pzuraq and all of y'all that helped hone this proposal!

🚁 🐎 🚗

@rwjblue rwjblue merged commit f5aa0b8 into master Jun 22, 2020
@rwjblue rwjblue deleted the invoke-helper branch June 22, 2020 16:42
@chriskrycho
Copy link
Contributor

Note: OP needs an update to the final version of the RFC!

@lougreenwood
Copy link

lougreenwood commented Sep 7, 2020

I wonder if the RFC should mention/clarify that this doesn't mean that helpers now replace utils (or perhaps it does) - in many cases a helper can also be useful as a util for re-use in some JS. At the moment it makes sense to write the helper as a util and then call the util from the helper:

utils/my-reusable-util.js

export default function myReusableUtil(someService, thingy) {
  return someService.modifyThingy(thingy)
}

helpers/my-reusable-util.js

import Helper from '@ember/component/helper';
import myReusableUtil from '../utils/helper';
import { inject as service } from '@ember/service';

export default class MyReusableUtilHelper extends Helper {
  @service someService;

  compute([thingy]) {
    return myReusableUtil(this.someService, thingy);
  }
}

However this PR could encourage the perspective that most/all utils should just always be helpers, since helpers can be easily invoked in JS or templates - so why bother with utils at all after this?

rwjblue added a commit to ember-cli/babel-plugin-ember-modules-api-polyfill that referenced this pull request Sep 29, 2020
Adds Helper Manager and `invokeHelper` APIs from emberjs/rfcs#625 and
emberjs/rfcs#626.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants