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

Fix remote relations #32

Closed
wants to merge 25 commits into from
Closed

Conversation

glesage
Copy link
Contributor

@glesage glesage commented Jan 8, 2016

It seems the remote datasource connector does not allow include: relation

I've written a few tests that currently show how it does not work... though I have yet to figure out how to fix it.

I was hoping it would show "failing tests" under the github status

@slnode
Copy link

slnode commented Jan 8, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@glesage
Copy link
Contributor Author

glesage commented Jan 8, 2016

Its not actually supposed to pass currently... the 1 test that matters fails, not sure how or why or how to fix it but I will attempt.. and welcome any tips (:

We really need this at the company I work at...

This https://github.com/glesage/loopback-connector-remote/blob/fix-remote-relations/test/relations.test.js#L60 fails

@glesage glesage closed this Jan 8, 2016
@glesage glesage deleted the fix-remote-relations branch January 8, 2016 19:45
@drywolf
Copy link

drywolf commented Jan 10, 2016

I'm currently also looking into this bug, what I did for a experimental fix was to add the following code to the relations mixin (at https://github.com/strongloop/loopback-connector-remote/blob/master/lib/relations.js#L209)

  var scope = function() {
      var cached = that.__cachedRelations[def.name];

      if (arguments.length == 0)
          return cached;

    return that['__get__' + def.name].apply(that, arguments);
  };

it simply checks if the relation property function is being called without any parameters and returns the cached data if it is available.
I'm not very experienced with the code though, so I can't tell if this is the right way to fix this issue or if the __cachedRelations should be fetched in a different place instead.

@glesage
Copy link
Contributor Author

glesage commented Jan 10, 2016

Hm ok awesome, I'm not either... I've actually got a current pull request open with a failing test to demonstrate the issue exactly...

I'll run it with your fix see if the issue is resolved with this.. temporarily

#33

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.

4 participants