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

Usage without mixins #139

Closed
josemarluedke opened this issue Jul 5, 2018 · 8 comments · Fixed by #262
Closed

Usage without mixins #139

josemarluedke opened this issue Jul 5, 2018 · 8 comments · Fixed by #262

Comments

@josemarluedke
Copy link
Member

When using ES Classes and/or TypeScript, it is not the best approach to use mixins. I know is possible, using class XYZ extends Component.extend(Mixin) { ... but it would be nice to not have an alternative to using mixins.

@bgentry
Copy link
Member

bgentry commented Jul 6, 2018

Any other ideas for how to track watchQuery subscriptions on a per route/component/object basis? And to auto-unsubscribe when the object is torn down?

@josemarluedke
Copy link
Member Author

Great discussion about a similar subject on Ember Simple Auth: mainmatter/ember-simple-auth#1619

@mike-north
Copy link
Contributor

mike-north commented Jul 9, 2018

Looking at the mixins in question, it doesn't seem all that difficult for those who want a more explicit "composition instead of inheritance" option.

Example:

https://github.com/bgentry/ember-apollo-client/blob/5296032ff0b10ac3bd9bebf6cca8f56946c90929/addon/mixins/route-query-manager.js#L5-L8

99% of the complexity in this addon lives on the service, which is excellent. It's very much true that a Router/Component/Object has an object that provides apollo support rather than being an object that includes apollo support.

If you don't want to use the RouteQueryManager mixin, just inject the service:apollo yourself, and add the appropriate one-liners to your route (or a route base class).

For a first-class TS/ES6 approach, looks like the work would be:

  • document the mixin-free approach -- effectively doing what the respective mixins do
  • add a few acceptance tests (A route, component and object) to protect against regression. The main thing that could go wrong in the future is ordering setup/teardown of the apollo client w/ the rest of the setup/teardown logic. As it stands, I doubt it matters if you do the apollo work before or after this._super(...arguments); but that could change. Tests would alert us to that change.

@bgentry
Copy link
Member

bgentry commented Jul 18, 2018

Since we're discussing a major version bump due to #145, it'd be great for that new version to also include a non-mixin / more typescript-compatible (#140) way of using this addon. In fact if we can introduce such an option even sooner, then we potentially deprecate the mixins and drop them from the new major version.

That being said, I read through the ember-simple-auth issue again and I have some concerns about whether it'd be just as easy to drop the mixins from ember-apollo-client.

The main distinction I see is that ember-simple-auth typically has very few integration points within a user's app, so the amount of code duplication from dropping mixins is low. ember-apollo-client, however, is commonly used by injecting the mixin all over the place: routes (just a base route in my case), components, etc. I'm less excited about dropping the mixins if it means I have to either a) write my own mixin, or b) duplicate code within my many components that use ComponentQueryManager. Or am I misunderstanding something here?

@mike-north
Copy link
Contributor

To be clear, we're not aiming to drop mixins from ember-simple-auth -- just to move most of the complexity away from the mixins (as you already have done) so that the project can be consumed in a mixin-free way without a lot of copy/pasting from source

@buschtoens
Copy link
Contributor

buschtoens commented Oct 14, 2018

I've never worked with ember-apollo-client before, but am currently evaluating it. This was one of the first problems that sprung to mind for me and I am happy to see, that this is already being discussed.

I couldn't put it better as mainmatter/ember-simple-auth#1619 (comment):

I personally would like to see a decorator approach. I think it makes it explicit and easy to reason about.

Using a class decorator like @withApollo seems reasonably clean to me:

import Component from '@ember/component';
import { withApollo, QueryManager } from 'ember-apollo-client';

@withApollo
export default class ExampleComponent extends Component {
  apollo!: QueryManager;
}

One current drawback of decorators in TypeScript is that they cannot yet mutate the type signature (microsoft/TypeScript#4881), which forces the user to manually type the apollo property. This is only a temporary issue though.

Instead of making this a class decorator, we could also use a class property decorator, like:

import Component from '@ember/component';
import { queryManager, QueryManager } from 'ember-apollo-client';

export default class ExampleComponent extends Component {
  @queryManager apollo!: QueryManager;
}

This would be similar to how service injections work:

import Component from '@ember/component';
import RouterService from '@ember/routing/router-service';
import { service } from '@ember-decorators/service';

export default class ExampleComponent extends Component {
  @service router!: RouterService;
}

This would also logically couple the QueryManager "injection" to the property that is used as the target for the injection, meaning that user could use a different property name, if they wanted to:

import Component from '@ember/component';
import { queryManager, QueryManager } from 'ember-apollo-client';

export default class ExampleComponent extends Component {
  @queryManager store!: QueryManager;
}

I don't know, if this is a good thing, but I like the symmetry with @service and that you can see at a glance what injection is provided by using the decorator. Also makes it a bit easier to remember to actually use the decorator when declaring the apollo property, while using @withApollo visually and mentally separates the two.

Anyhow, another advantage is that we can use the same decorator for all types of objects, since the decorator can query the type of the decorated class.

@tchak
Copy link

tchak commented Dec 3, 2018

I implemented queryManager macro on my fork if some of you are interested in trying it out and give feedback. cc @buschtoens

https://github.com/tchak/ember-apollo-client/tree/drop-mixins

@josemarluedke
Copy link
Member Author

@tchak That approach seems very interesting. I will give it a try this week and see how it goes.

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

Successfully merging a pull request may close this issue.

5 participants