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

delegate is not a function #11592

Closed
rlivsey opened this issue Jun 29, 2015 · 34 comments
Closed

delegate is not a function #11592

rlivsey opened this issue Jun 29, 2015 · 34 comments
Assignees
Labels

Comments

@rlivsey
Copy link
Contributor

rlivsey commented Jun 29, 2015

I just updated our app to try the latest Canary and it appears to break query params for us.

We're seeing:

Error while processing route: xxx delegate is not a function TypeError: delegate is not a function

Here's the backtrace etc…

screenshot 2015-06-29 17 31 10

I'll see if I can reproduce in a smaller app asap.

@rwjblue
Copy link
Member

rwjblue commented Jun 29, 2015

I'll see if I can reproduce in a smaller app asap.

Definitely would be helpful.

@rlivsey
Copy link
Contributor Author

rlivsey commented Jun 30, 2015

Managed to reproduce in JSBin, it looks to be if the query param is a computed property:

http://emberjs.jsbin.com/dusufa

@rwjblue
Copy link
Member

rwjblue commented Jul 1, 2015

This might be related to moving the config to routes.

@trek / @machty - Could one of y'all take a look when you have a free minute?

@trek trek self-assigned this Jul 7, 2015
@machty
Copy link
Contributor

machty commented Jul 7, 2015

@rlivsey what's your use case for using a computed property as a QP's default value? I honestly don't expect that to work, and if it did before, it might have been an accident, but I'd like to understand how you are using it in your app.

@rlivsey
Copy link
Contributor Author

rlivsey commented Jul 7, 2015

My use-case is to display a formatted Date in the URL, so I was using a computed property for the string form:

const DATE_FORMAT = 'YYYY-MM';
export default Ember.Controller.extend({
   queryParams: {
     dateStr: "date"
   },

  date: null,

  dateStr: Ember.computed("date", {
    get() {
      const date = this.get("date");
      return date ? date.format(DATE_FORMAT) : null;
    },
    set(k, v) {
      this.set("date", window.moment.utc(v, DATE_FORMAT));
      return v;
    }
  })
});

I suppose instead I could instead invert this behaviour and instead of dateStr being the computed property I can make date set it instead:

  dateStr: null,

  date: Ember.computed("dateStr", {
    get() {
      const dateStr = this.get("dateStr");
      return dateStr ? window.moment.utc(dateStr, DATE_FORMAT) : null;
    },
    set(_, date) {
      this.set("dateStr", date ? date.format(DATE_FORMAT) : null);
      return date;
    }
  })

This works, but it would be nice to have the option to do it the other way. If using a CP for the value, it could just assume the default is null?

@amokan
Copy link

amokan commented Aug 21, 2015

This has burned me on the 2.0.0 release as well. Had to revert back to 1.13.8 because I have a lot of this sort of thing going on.

@atsjj
Copy link

atsjj commented Aug 25, 2015

Thought I'd mention I just ran into this too. Similar story to @rlivsey...

@rlivsey
Copy link
Contributor Author

rlivsey commented Aug 25, 2015

I ran into this again the other day, similar use-case where I want the query string to reflect the current selected person in a list. The most straightforward approach is to simply alias the query param to the selected person's ID:

export default Ember.Controller.extend({
  queryParams: {
    selectedID: "selected"
  },
  selected: null,
  selectedID: Ember.computed.reads("selected.id")
});

That doesn't work, so now I'm forced to make this more complex and should also leave a comment to make sure a colleague doesn't try and refactor it back to the simpler version:

export default Ember.Controller.extend({
  queryParams: {
    selectedID: "selected"
  },  
  selectedID: null,

  // query params doesn't support computed properties, so set the ID manually
  // see issue for more details: https://github.com/emberjs/ember.js/issues/11592
  selected: Ember.computed({
    set(_, person) {
      this.set("selectedID", person ? person.get("id") : null),
      return person; 
    }
  })
});

@lolmaus
Copy link
Contributor

lolmaus commented Sep 2, 2015

Possibly related: #12102.

@kmiyashiro
Copy link

I am also getting this error after upgrading from 1.13.8 to 2.0.2. I'm not even using query param aliases and they are normal properties.

I noticed that it only occurs when I set the query params on('init', ...). This used to work fine in 1.13.x.

controller:

  queryParams: ['from', 'to', 'period', 'preset'],
  // Default date range
  preset: 'past 7 days',
  period: 'day',
  from: 0,
  to: 0,
...
  setDatesFromPreset: on('init', observer('preset',
    function setDatesFromPreset() {
      const preset = this.get('preset');
      const {
        from,
        to,
        period,
      } = ReportController.datePresetQueryParams(preset);

      if (!isNone(from) || !isNone(to)) {
        this.setProperties({
          period,
          _from: from,
          _to: to,
        });
      }

      this.set('toggleLabel', Ember.String.capitalize(preset));
    })
  ),

@lolmaus
Copy link
Contributor

lolmaus commented Oct 8, 2015

So are query params in Ember 2.x effectively broken? 😕

PS What's Canary in subj's title is now Stable.

@jcope2013
Copy link
Contributor

having same issue

@Sinled
Copy link

Sinled commented Oct 18, 2015

Also have a problem (in all browsers) with query params and computed property with get\set specified.

@topaxi
Copy link
Contributor

topaxi commented Oct 19, 2015

I just ran into this issue too.

@oliverwilkie
Copy link

+1

@nightire
Copy link
Contributor

Have been there: mainmatter/ember-simple-auth#743

@Glennvd
Copy link

Glennvd commented Oct 28, 2015

+1

1 similar comment
@lenitama
Copy link

+1

@trek trek changed the title Canary - delegate is not a function delegate is not a function Oct 31, 2015
@trek
Copy link
Member

trek commented Oct 31, 2015

This came up in the Core Team discussion for 2015-10-23.

Initial default values on query params were always intended to be statics but the language didn't give us the ability to express that.

There weren't any tests around using a computed property here because nobody thought people would use them in this way. As @machty said:

I honestly don't expect that to work, and if it did before, it might have been an accident, but I'd like to understand how you are using it in your app.

Basically, using a computed property in this way only worked coincidentally in some cases and a change has removed those cases. However, using a computed property this way likely introducing insidious edge-casey bugs for some interactions between a query param and controller because the the value was being looked up on the controller's prototype, not the controller single instance itself.

See here and here for the relevant code before the changes.

We'd accept a PR for 1.x that fixes if it also included enough tests demonstrating that there aren't any weird interactions between query params / controller when the query param property is a computed property. We could include this in the next 1.13.x release for people migrating.

We wouldn't carry this behavior over to 2.0 because it definitely won't be possible with the various upcoming router changes.

@rlivsey's solution should allow people to avoid this #11592 (comment)

@trek trek closed this as completed Oct 31, 2015
@rlivsey
Copy link
Contributor Author

rlivsey commented Oct 31, 2015

Fair enough!

Would be good to have a nice descriptive error message pointing people in the right direction & making it clear it's not a bug.

I'll take a stab at this shortly.

Any pointers / advice on how to detect a CP being used in this way would be great.

@lolmaus
Copy link
Contributor

lolmaus commented Oct 31, 2015

For anyone interested, I've tried a basic example on JSBin and it seems to work fine:

http://emberjs.jsbin.com/cevahu/1/edit?html,js,output

@bschnelle
Copy link

+1

@DylanLukes
Copy link

Also experiencing this. To give a little more detail for my particular case, it seems that at the time setup is called on the controller using the remote pagination, _qpDelegate is null.

The sequence of events I can so far infer is:

  • Route receives model from Ember.Data/jqXHR.
  • ???
  • Route updates queryParams to default values (why is this happening model fetching?).
  • setup gets called on each controller (this is the method that finishes up by calling setupController)
  • ???
  • _qpDeletegate property of controller is undefined -> error happens.

Note: I'm not using any computed properties on my Route. I am using ember-cli-pagination though. This bug occurs when setting the an initial static value.

@nathanhammond
Copy link
Member

/cc @luisah

@knownasilya
Copy link
Contributor

So from Slack @rwjblue was suggesting writing a QPs service that basically allows you to subscribe to changes on a single QP in that service. Would love to flesh this out and maybe write an addon and RFC.

@mellatone
Copy link
Contributor

+1 just experienced this upgrading to 2.x. @knownasilya I'm down to help.

@knownasilya
Copy link
Contributor

@2468ben
Copy link

2468ben commented Apr 8, 2016

@knownasilya I've been using a similar QP setup (service + mixins) in my company's main app since 2.0, and I would love to use your addon instead and help with the RFC.
I have a ton of URL-related features (good ideas and bad ideas, but all with tests), so hopefully it'll be a nice collection of edge cases you can run through.

@knownasilya
Copy link
Contributor

@2468ben would love to have you on board. Feel free to submit issues/PRs.

@cibernox
Copy link
Contributor

I just found this too in my app. How can I help? Reproducible example?

@herom
Copy link

herom commented May 18, 2016

I'm too seeing this in my application. I recently upgraded from Ember 1.13.10 all the way up to Ember 2.4.3 and now some of the routes are broken, especially when reloading them...

@sergiferran
Copy link

sergiferran commented Jun 16, 2016

What i've found, is that if you run something in the controller (like this.set('something', someValue) before the route finishes the setupController, then it fails.
I hope this helps to you to fix it.

@lbblock2
Copy link

Any recommendations for fixes using the remote paginated API?

@kellyChex
Copy link

kellyChex commented Sep 8, 2016

+1

thought I'd also share my specific problem and workaround 😁

In our application, we have two date search boxes, ‘From Date’ and ‘To Date.’ When a user visits the page, 'From Date' and 'To Date' display the date range of our applications' custom current cycle. The 'From Date' and 'To Date' change based on the current date thus, the calculations are done in a service. 'From Date' and 'To Date' are also query parameters, set in the controller, for export purposes and because our users use this detailed link. As a workaround, I used the setupController in a route and set the 'from' and 'to’ to grab the calculations from the service.

export default Ember.Route.extend({
  nameOfService: Ember.inject.service('name-of-service'),
  setupController: function(controller, model){
    this._super(controller, model);
    var nameOfService = this.get('nameOfService');
    controller.set('from', nameOfService.fromDate());
    controller.set('to', nameOfService.toDate());
  }
});

rauhryan added a commit to rauhryan/ember.js that referenced this issue Oct 31, 2016
before mapping all the queryParams. 

In certain scenarios, if query params are shared 
through a service, bound query params will 
fire the _qpDelegate while transitioning to a new
controller if the property has been modified from
it's default value. 

Loosely related to 
  - emberjs#11592

Based on a previous PR 
  - emberjs#13981
rauhryan added a commit to rauhryan/ember.js that referenced this issue Nov 9, 2016
before mapping all the queryParams.

In certain scenarios, if query params are shared
through a service, bound query params will
fire the _qpDelegate while transitioning to a new
controller if the property has been modified from
it's default value.

Loosely related to
  - emberjs#11592

  Based on a previous PR
    - emberjs#13981
chancancode pushed a commit that referenced this issue Nov 27, 2016
before mapping all the queryParams.

In certain scenarios, if query params are shared
through a service, bound query params will
fire the _qpDelegate while transitioning to a new
controller if the property has been modified from
it's default value.

Loosely related to
  - #11592

  Based on a previous PR
    - #13981

(cherry picked from commit 8f1db44)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests