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

Check if adapter#query is a wrapped function #5345

Merged
merged 1 commit into from Mar 28, 2018

Conversation

ryanto
Copy link
Contributor

@ryanto ryanto commented Jan 31, 2018

Currently the if statement always returns false because adapter#query is a wrappedFunction, which has length == 0.

This PR checks if the query is a wrappedFunction and if it is we'll check the length of that function.

I'm also wondering if we can get rid of the if statement and have the recordArray created and given to the query function in all cases. My guess is that the conditional guards around having to create and then later add models to the record array because there is a problem with the events that fire? @stefanpenner do you have any thoughts?

@bmac bmac requested a review from stefanpenner March 8, 2018 16:29
@bmac
Copy link
Member

bmac commented Mar 8, 2018

@stefanpenner do you have some time to look at this pr?

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

tldr; any function that gets super wrapped cannot be checked for arity (because the super wrapper doesn’t have any arguments specified).

@ryanto
Copy link
Contributor Author

ryanto commented Mar 9, 2018

Fyi, I turned out not needing this. We ended up using a different approach. Not sure if the PR would help anyone else though.

@runspired
Copy link
Contributor

We discussed this at the weekly meeting today and roughly our opinion is this:

  • we should fix the current intent in the near term, which means merging this PR
  • this check is likely to be brittle, especially since Ember will soon be moving the superWrapper into a WeakMap where we would not be able to check it, in the not so long term we need to remove the check
  • to remove it, we need to cleanup the SnapshotRecordArray / adapterOptions mess, this arity check is present to enable us to often avoid a costly allocation
  • we are currently plotting an overhaul of the request layer, which will allow us to plot a path out of this craziness

@runspired runspired merged commit 9875086 into emberjs:master Mar 28, 2018
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

Successfully merging this pull request may close these issues.

None yet

4 participants