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

Ability to inject service into Template Only components #543

Closed
mehulkar opened this issue Sep 13, 2019 · 15 comments
Closed

Ability to inject service into Template Only components #543

mehulkar opened this issue Sep 13, 2019 · 15 comments

Comments

@mehulkar
Copy link
Contributor

mehulkar commented Sep 13, 2019

Templates that want to use functionality from services are currently forced to create a backing JS class for the injection. The downside of this is that once the class exists, it can be easy to stuff functionality into it. To keep simple components simple, it would be helpful to be able to do injections some other way.

It's possible that the template import primitives discussion in #454 can be part of this discussion.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 13, 2019

I definitely agree that TO-components will pretty frequently want services in order to be a bit more flexible, and have been thinking of the best way to do this. There are 2 main possibilities that I think would be pretty straightforward.

Service Injections in Helpers and Modifiers

This is technically already possible today, with class based helpers, and with service injections in ember-functional-modifiers. This is also one of the more likely places that users will want to introduce services, so I think it would work out. You could imagine with a possible template-import syntax being able to do something like this:

import { helper } from '@ember/helper';

const isDarkMode = helper((theme) => {
  return theme.isDarkMode;
}, { services: ['theme'] })

const toggleDarkMode = helper((theme, [event]) => {
  theme.isDarkMode = event.target.checked;
}, { services: ['theme'] })

export default <template>
  <div class="{{if (isDarkMode) 'dark'}}">
    Dark Mode: 

    <input type="checkbox" {{on "change" toggleDarkMode}} />
  </div>
</template>

You could also imagine creating a generic service helper:

import Helper from '@ember/helper';

class service extends Helper {
  compute([serviceName]) {
    return getOwner(this).lookup(`service:${serviceName}`);
  }
}

export default <template>
  <div class="{{if (get (service 'theme') 'isDarkMode') 'dark'}}">
    Dark Mode: 

    <input 
      type="checkbox" 
      {{on "change" (set (service 'theme') 'isDarkMode (get _ 'target.checked'))}} 
    />
  </div>
</template>

A bit verbose, but not bad!

Services in Argument Defaults

The other option I think would be to allow injecting services when providing defaults to arguments. This would require us to figure out how to specify argument defaults (see #479 for more discussion on that).

I'm not so much a fan of this approach, because then it wouldn't be easy to tell if an argument was actually an argument, or was a service that was being injected. I'd be worried that folks would try to inject services on class based components this way as well.

Other Options?

I can't really think of a simple/straightforward way to do this otherwise, but maybe we can think outside the box a bit here. It could be a new syntax for referencing services (e.g. the way @arg references the arg named arg, we could have a way to directly reference services). I think the key thing is figuring out how to reference them without using @ syntax or this, since the first would be confusing, and the second isn't available in TO components.

Thanks for opening this discussion topic btw! I think this is a very important thing to figure out 😄

@buschtoens
Copy link
Contributor

buschtoens commented Sep 13, 2019

We use this helper:

import { getOwner } from '@ember/application';
import Helper from '@ember/component/helper';
import { assert } from '@ember/debug';

export default class ServiceHelper extends Helper {
  compute([serviceName]: [string]) {
    const service = getOwner(this).lookup(`service:${serviceName}`);
    assert(`The service '${serviceName}' does not exist`, service);

    return service;
  }
}

It allows you to do stuff like:

{{get (service "user-agent") "isNativeApp"}}

Edit: @pzuraq beat me to the punch 😄

Edit: I published the {{service}} helper as an addon: ember-service-helper

@pzuraq
Copy link
Contributor

pzuraq commented Sep 13, 2019

@buschtoens It's really cool to see that y'all are using it in real world apps though! Always great to design with actual feedback from usage, definitely curious to hear how it works out for your use cases, if you care to talk about it.

@buschtoens
Copy link
Contributor

@pzuraq Sure! I'll ping you in Discord, when I have some time. 😊

Previously we were using it like this:

export default class ServiceHelper extends Helper {
  compute([serviceName, propertyName]: [string, string]) {
    const service = getOwner(this).lookup(`service:${serviceName}`);
    assert(`The service '${serviceName}' does not exist`, service);
    assert(
      `The property '${propertyName}' does not exist on the ${serviceName} service`,
      propertyName in service
    );
    return service[propertyName];
  }
}
{{service "user-agent" "isNativeApp"}}

Which was a bit less verbose, but had two significant down-sides:

  • no nested property access, e.g. {{get (service "user-agent") "browser.version")}}
  • changes are not observed 😱

It was a quite nice experience to fallback to the "do one thing well" philosophy, and let {{get}} handle the property access. 😇

So far we've only been using the helper to read properties, but theoretically it could also be used to set properties, using your ember-set-helper, or to call methods. When calling methods, it's important to decorate them with @action or alternatively use the ember-bind-helper.

@Panman82
Copy link

Panman82 commented Sep 13, 2019

I had the same idea of using a helper. And, you could use it in conjunction with {{let}} to reference it multiple times and/or later-on. Which would work nicely with @mehulkar other RFC issue/idea #520

@buschtoens
Copy link
Contributor

I published ⬆️ {{service}} helper as an addon: ember-service-helper

@mehulkar
Copy link
Contributor Author

Using a helper is a great idea! My concern was that it adds another level of indirection, but I didn’t think to name my helper service so that it would be more obvious.

That said, I think my original idea was something more aligned with app.inject. I generally think this API needs to be given a little more of the lime light, but not sure what the right answer here is.

Copy link
Member

rwjblue commented Sep 17, 2019

IMHO, app.inject should be deprecated and removed (there is another open issue here for that specifically).

@mehulkar
Copy link
Contributor Author

@rwjblue are you talking about #508? Didn't see anything else searching with "inject" that was relevant. Would be interested to read that proposal/discussion because on the surface, I think app.inject is a good primitive if DI is going to continue to be a thing. Based on the current programing model and seeing people come into Ember, I'd want to more explicit, if anything. (But I realize that this is a totally separate discussion and we should have it elsewhere!)

@mehulkar
Copy link
Contributor Author

Thought about this more. I think the helper approach is great, but I have two concerns:

  • it's asymmetrical to have to go through a helper, compared to injections in JS
  • because it's not provided by core, it seems to do a disservice to Template Only Components, which I think are the way forward.

So it's possible that the RFC needed here is to add this helper to core, but that still leaves the first issue of asymmetry.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 17, 2019

@mehulkar do you have a particular syntax that you would like to see? Even a rough gist would help here, it’s hard to talk about this in the abstract.

@mehulkar
Copy link
Contributor Author

mehulkar commented Sep 18, 2019

Something like this looks symmetrical-ish and makes sense to me:

{{let (lookup "service:foo-bar") as |FooBarService|}}
  <button {{on "click" FooBarService.doSomething}}>Click</button>
{{/let}}

This is already accomplishable of course, but then maybe the RFC is to make the lookup helper a part of core?

On the question of symmetry, another q that comes up (and may already be answered) is that after #451, now that owner is easily available, is there value in:

class Foo extends Service {
 @service bar;
}

vs

class Foo extends Service {
  constructor(owner) {
    super(...arguments);
    this.bar = owner.lookup('service:bar');
  }
}

If the latter is the way forward, then a lookup helper in core for templates makes even more sense.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 18, 2019

To clarify, it definitely was not our intent to make the later example the recommended way going forward. In fact, the reason owner is passed as the first parameter is to enable service decorations to work, since there isn't really a way to pass the owner out of band safely.

The reason decorators are better for defining injections is that they are inherently less dynamic. Using lookup directly, you could do something like this:

class Foo extends GlimmerComponent {
  constructor(owner, args) {
    super(...arguments);
    this.bar = args.useBar1 ? owner.lookup('service:bar1') : owner.lookup('service:bar2');
  }
}

This means that to fully know what the service is intended to be, you will have to read the entire constructor (or potentially the entire class definition, if users decide to lookup/assign services during different lifecycle hooks). Using a decorator, we can know that bar will always a specific service.

This is much easier for users to read through and analyze, and for bundlers to read through and analyze (for a future world where we have Embroider, and the ability to bundle and lazy load assets more easily).

In general, the recommendation is to avoid using the owner directly, and to simply pass it through so it's available on the class itself. I would personally avoid a lookup helper for similar reasons. I would be more open to a service helper, though I think it would have some of the same problems.

@mehulkar
Copy link
Contributor Author

That makes sense and seems reasonable to me. If lookup is not the happy path in JS, then I don't think recommending it in a template makes sense either. A service helper is mostly syntactic sugar, so it just seems like a thinly veiled footgun than the footgun itself :)

Soooo, I guess that leaves us back at square 1, with the options of:

  • special syntax for template injections (something comparable to decorator approach in JS)
  • be ok with asymmetry and use a helper (either in core or user land)

@mehulkar
Copy link
Contributor Author

It doesn't seem like there is a good path forward here that's different from the more generalized Template Imports in #454, so I'm closing this down. For the current use case a helper from userland works fine. Ergonomically, now that we have template colocation, I'm also ok with having a JS class for the sole purpose of service injection. I'm not sure a separate primitive in templates really solves any real problem. Thanks all for the discussion!

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

No branches or pull requests

5 participants