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

Accessing the controller / services #46

Closed
buschtoens opened this issue Nov 21, 2017 · 17 comments
Closed

Accessing the controller / services #46

buschtoens opened this issue Nov 21, 2017 · 17 comments

Comments

@buschtoens
Copy link
Contributor

Would it hurt, if we were able to access the corresponding Controller from the serialize and deserialize methods?

I am asking primarily because I need to synchronously access the store service, in order to lookup a record for an ID. Basically something like this:

new QueryParams({
  someModel: {
    defaultValue: null,

    serialize(model) {
      return model ? get(model, 'id') : '';
    },

    deserialize(id) {
      return id ? get(this, 'store').peekRecord('model-name', id) : null;
    }
});

Do you think this is a form of unhealthy entanglement, even though the lookup is guaranteed to work synchronously?

@offirgolan
Copy link
Owner

@buschtoens I think thats a pretty valid use-case and can be easily done by passing the controller to those methods here and here.

The only concern that I have is that when the app boots, if the controller is not yet ready we use the proto to access some methods / attributes. At that point, I'm not really sure if you will have access to the store or any other service tbh. I think this is something that should be pretty well tested.

PR is welcome!

/cc @poteto

@poteto
Copy link
Collaborator

poteto commented Nov 22, 2017

I've had that thought before.

As @offirgolan says It's definitely possible, although I would pass the controller in as an argument rather than have the context of those functions be rebound. I think having this in the serialize or derserialize functions be the controller would be slightly confusing

@Techn1x
Copy link

Techn1x commented Jan 9, 2018

+1!

Seems like a very common use case - having a record ID (or array of them) in the URL, then wanting to easily retrieve those records

@Techn1x
Copy link

Techn1x commented Jan 9, 2018

I gave this a quick go by passing in the controller to both methods as suggested above, and it seems like the controller isn't always ready and when it's not, there's no access to the store.

Any other alternatives?

@buschtoens
Copy link
Contributor Author

@poteto Absolutely. How would you like the controller to be passed? As a a second param directly or wrapped in an object, so that it's easier to add additional props in the future?

new QueryParams({
  someModel: {
    defaultValue: null,

    serialize(model, controller) {
      return model ? get(controller, 'id') : '';
    },

    deserialize(id, controller) {
      return id ? get(controller, 'store').peekRecord('model-name', id) : null;
    }
});
new QueryParams({
  someModel: {
    defaultValue: null,

    serialize(model, { controller }) {
      return model ? get(controller, 'id') : '';
    },

    deserialize(id, { controller }) {
      return id ? get(controller, 'store').peekRecord('model-name', id) : null;
    }
});

@offirgolan
Copy link
Owner

My vote is for the first option @buschtoens. I dont think there will be any other future arguments since everything you need should be on the controller 😛

@poteto
Copy link
Collaborator

poteto commented Jan 12, 2018

First option looks good!

@buschtoens
Copy link
Contributor Author

Cool!

@Techn1x Do you want to land a PR? 😉
If not I'll get around to it next week.

@Techn1x
Copy link

Techn1x commented Jan 15, 2018

I'll see if I can find the time this week

@evoactivity
Copy link

I don't seem to be able to access the store at all.

I get an Assertion Failed: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container. error on get(controller, 'store') .

@buschtoens I assume your PR worked for your use case or did you need to do anything else to actually access the store?

@buschtoens
Copy link
Contributor Author

Yeppa. Worked. 😅

Are you doing anything special? Can you setup a demo? Does the error only occur when you try to get(controller, 'store') or does it occur independently of it?

@evoactivity
Copy link

evoactivity commented Jun 27, 2018

edit 2: I was able to setup a twiddle, where it seems to work. Twiddle seems to clear out the url parameters onload for some reason, but it seems to work.

I've done it all on the application route, so going to try a nested route so it more closely resemble my actual app.

edit 3: Putting it into a new route seems to recreate the issue,
https://ember-twiddle.com/aab5e0a29a80869b3c1dbf6915e86094?openFiles=templates.application.hbs

Original:
Doing get(controller, 'someOtherProperty') works if it's just a normal property on the controller, it's only when I try to get a service.

this is my parameter

selectedQualifications: {
  as: 'qualifications',
  defaultValue: [],
  serialize(records) {
    let ids = records.map((record) => {
      return get(record, 'id');
    });
    return ids.toString();
  },
  deserialize(ids = '', controller) {
    let records = [];
    ids.split(',').forEach((id) => {
      let record = get(controller, 'store').peekRecord('course', id);
      records.addObject(record);
    });

    return records;
  }
}

If the page loads without the query parameter set it seems to work (because it's not trying to deserialize it I suppose), if the parameters are set when the page loads then it errors.

This is on ember 2.18 (same for ember-data).

@roberkules
Copy link

roberkules commented Jun 27, 2018

I have the same issue

"Assertion Failed: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container."

pretty much same same code:

deserialize(value = '', controller) {
  let store = get(controller, 'store');  // <== FAILS HERE
  return value.split(',').map(id => store.peekRecord('location', id));
}

I'm on:

Ember:           3.0.0
Ember Data:      3.0.2
ember-parachute: 0.3.7

@offirgolan
Copy link
Owner

Assertion Failed: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container. error on get(controller, 'store')

This is stating that the controller has not yet been initialized with the container necessary for you to access the store (or any other service). This is the one caveat with this implementation in that serialize and deserialize can be called before the controller is actually ready. Unfortunately, this isn't something that can be fixed from this addon.

@roberkules
Copy link

@offirgolan @buschtoens so how would you deal with scenarios where you have a record/recordarray that needs to be persisted to the url? any advice?

@evoactivity
Copy link

evoactivity commented Jun 28, 2018

@roberkules I'm going to try an aproach today using a computed property which is the actual records array with the url parameters just being an array of ids, when that array updates the computed property should recompute a corresponding array.

@offirgolan @buschtoens would you say that sounds like a reasonable way to implement this functionality?

edit: turns out I didn't need a computed property at all.

In the setup() hook I added

queryParams.chosenQualificationIds.forEach((id) => {
    let record = this.store.peekRecord('course', id);
    get(this, 'selectedQualifications').addObject(record);
});

Then I'm using an ember-power-select component to modify the array, instead of just doing onchange=(action (mut selectedQualifications)) I used an actions to modify both properties

actions: {
    setQualifications(records) {
      let ids = records.map((record) => record.id);
      set(this, 'chosenQualificationIds', ids);
      set(this, 'selectedQualifications', records);
    }
}

My query parameter maps to chosenQualificationIds

@Techn1x
Copy link

Techn1x commented Mar 14, 2019

Thanks @evoactivity I also added queryParamsDidChange so that even if the ID in the URL is changed later, those changes will be shown

export const validQueryParams = new QueryParams({
  selectedAssetId: {
    as: 'asset',
    defaultValue: ''
  }
});
// Set initial selectedAsset from ID in URL
setup({ queryParams }) {
  if(queryParams.selectedAssetId) {
    this.store.findRecord('asset',queryParams.selectedAssetId).then((asset) => {
      this.set('selectedAsset',asset));
    }
  }
},

// If ID in URL changes, update the selectedAsset
queryParamsDidChange({ shouldRefresh, queryParams, changed, changes }) {
  if(queryParams.selectedAssetId !== this.get('selectedAsset.id')) {
    if(queryParams.selectedAssetId) {
      this.store.findRecord('asset',queryParams.selectedAssetId).then((asset) => {
        this.set('selectedAsset',asset));
      }
    } else {
      if(this.get('selectedAsset')) this.set('selectedAsset',undefined);
    }
  }
},

actions: {
  // Use this action from the template to set a new asset, so that both properties are modified
  selectAsset(asset) {
    this.set('selectedAsset',asset);	// Set the selectedAsset to use
    this.set('selectedAssetId', asset ? asset.get('id') : '');	// Set the asset ID in the URL
  }
}

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

6 participants