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

Duplicate identifier 'options' #310

Closed
4 tasks
wvanrensburg opened this issue Jan 10, 2017 · 24 comments · Fixed by #336
Closed
4 tasks

Duplicate identifier 'options' #310

wvanrensburg opened this issue Jan 10, 2017 · 24 comments · Fixed by #336

Comments

@wvanrensburg
Copy link

What type of issue are you creating?

  • [ X] Bug
  • Enhancement
  • Question

What version of this module are you using?

  • 2.0.10 (Stable)
  • [X ] 2.1.0-rc.8
  • Other

Write other if any:

Please add a description for your issue:

Ran the generator, included the necessary sdk, and got the following error for each method defined in every Service

error TS2300: Duplicate identifier 'options'.

Upon looking further, it appears that the options parameter is being duplicated in the method signature. Example of a built-in one from loopback

  public findByIdAccessTokens(id: any, options: any = {}, fk: any, options: any = {}): Observable<any> {
    let _method: string = "GET";
    let _url: string = LoopBackConfig.getPath() + "/" + LoopBackConfig.getApiVersion() +
    "/Users/:id/accessTokens/:fk";
    let _routeParams: any = {
      id: id,
      fk: fk
    };
    let _postBody: any = {};
    let _urlParams: any = {};
    let result = this.request(_method, _url, _routeParams, _urlParams, _postBody);
    return result;
  }

Notice that options has been duplicated. This is happening to every method in every Service. Any ideas?

@paulussup
Copy link
Contributor

paulussup commented Jan 10, 2017

Seems to be this commit in the loopback repo which has added the extra argument of options to the sharedCtor method. I can confirm using loopback v3.1.0 is a sufficient temporary solution.

 ModelCtor.sharedCtor = function(data, id, options, fn) { ...

@redaikidoka
Copy link

I'm still experiencing this in 2.1.0-rc.8.2

@jonathan-casarrubias
Copy link
Collaborator

Yes, this is actually a bug introduced by the framework itself not the builder. The builder analyzes all the parameters and the IBM guys actually added the options parameter as @paulussup is stating.

I need to know if that feature it is supposed to work with loopback 2 or not? if not I need to figure out a way to remove it from the options for loopback 2 users.

But as for now and to be honest I have no idea what the options is supposed to do and in which contexts should work.

@redaikidoka
Copy link

redaikidoka commented Jan 10, 2017 via email

@jonathan-casarrubias
Copy link
Collaborator

Cool, no worries I also asked @bajtos in which contexts should that work, lets wait for an answer.

Maybe the solution would be to set options as optional and verify it is not duplicated

@redaikidoka
Copy link

redaikidoka commented Jan 10, 2017 via email

@wvanrensburg
Copy link
Author

Thanks so much for quick responses. Downgrading to 3.1.1 did the trick for now!

@bajtos
Copy link
Contributor

bajtos commented Jan 12, 2017

Hello, first of all, notice that the option arguments added to LoopBack have a function as the http mapping. That means the argument is computed server-side and clients must not provide it.

Here is a code snippet from loopback-swagger showing how to correctly filter "accepts" arguments:

    // Filter out parameters that are generated from the incoming request,
    // or generated by functions that use those resources.
    accepts = accepts.filter(function(arg) {
      if (!arg.http) return true;
      // Don't show derived arguments.
      if (typeof arg.http === 'function') return false;
      // Don't show arguments set to the incoming http request.
      // Please note that body needs to be shown, such as User.create().
      if (arg.http.source === 'req' ||
        arg.http.source === 'res' ||
        arg.http.source === 'context') {
        return false;
      }
      return true;
    });

@bajtos
Copy link
Contributor

bajtos commented Jan 12, 2017

public findByIdAccessTokens(id: any, options: any = {}, fk: any, options: any = {})

IIUC, this method invokes two server-side methods under the hood: sharedCtor(id, options) used to resolve /api/Users/:id and then accessTokens(fk, options) to resolve remaining /accessTokens/:fk part. Notice that both function have the same argument "options" now.

IMO, LoopBack cannot guarantee that sharedCtor does not share any argument names with prototype methods. I think the SDK builder should be prepared to handle this situation and modify argument names, e.g. to options1 & options2 or ctorOptions & options:

public findByIdAccessTokens(id: any, ctorOptions: any = {}, fk: any, options: any = {})

bajtos referenced this issue in strongloop/loopback Jan 12, 2017
Modify remoting metadata of data-access methods in PersistedModel
and relation method in Model and add an "options" argument to "accepts"
list.
@bajtos
Copy link
Contributor

bajtos commented Jan 12, 2017

Cross-posting strongloop/loopback@60ea3e9#commitcomment-20427860:

But I'm totally sure this should work with LB2, it is supposed to? if so, I believe this may be an optional parameter, so if it is supposed to work with both LB2 and LB3 maybe the fix would be to set the options parameter to optional in typescript?

In LB2, the new "options" parameter is added to remoting metadata only when a model-level setting injectOptionsFromRemoteContext is set to true. So for existing apps upgrading to newer LB2 versions, the current SDK builder should keep working without any changes needed. However, once a model enables injectOptionsFromRemoteContext, the generated code will have the same issues as LB3 apps.

I believe this may be an optional parameter

In strong-remoting, all parameters are optional, unless their metadata specify required: true.

@bajtos
Copy link
Contributor

bajtos commented Jan 12, 2017

@jonathan-casarrubias I tried to give you as much information and wider context as possible in the comments above, I hope it will help you to better understand advanced features of strong-remoting metadata.

IMO, to fix this particular bug, you should filter out computed "accepts" arguments as shown in my #310 (comment). In the longer term, you may want to take into account the information from my other comments.

It makes me wonder, how can we capture this information in our docs so that future code-generator authors don't repeat this mistake? SDK builder is no the first project that forgot to handle computed arguments :)

@jonathan-casarrubias
Copy link
Collaborator

Hi @bajtos I really appreciate you taking the time to beautifully elaborate your. answer, I believe this information Is fair enough to solve this issue.

I was not totally sure if it was supposed to be exposed or not, but now it makes totally sense.

btw I would recommend to add a note here http://loopback.io/doc/en/lb3/Remote-methods.html that specifies these 3 http will not be available at client side. I believe it states other great information but didnt see anything about these options being exclusive for server side.

Again thanks a lot for elaborating your answer and I will be sending a fix for this asap.

Cheers
Jon

@bajtos
Copy link
Contributor

bajtos commented Jan 12, 2017

btw I would recommend to add a note here http://loopback.io/doc/en/lb3/Remote-methods.html that specifies these 3 http will not be available at client side. I believe it states other great information but didnt see anything about these options being exclusive for server side.

@jonathan-casarrubias that sounds like a good start to me. Would you mind sending a pull request with the changes yourself? In my experience, the documentation is written by the users is often the best, as the users know bests what they would expect to see in the docs. Just mention my handle @bajtos in the patch, so that I can review the proposal for correctness.

cc @crandmck

@jonathan-casarrubias
Copy link
Collaborator

@bajtos sure I can do that, I will fix this issue, update the LB documentation and send a pull request for the docs ASAP.

Cheers!
Jon

@redaikidoka
Copy link

redaikidoka commented Jan 12, 2017 via email

@rodplata
Copy link

Any updates on this?

josephsnow added a commit to BizVision/loopback-sdk-builder that referenced this issue Jan 20, 2017
Don't show internal server only remote method arguments in the client sdk declaration
ref mean-expert-official#310
@jonathan-casarrubias
Copy link
Collaborator

I'm sorry for the late response, this month is kind of crazy for me, I have 2 deliveries so its being difficult to work on these issues.

Anyway, what I love from the community is that I have been receiving the last week a bunch of pull requests that I'm right now integrating.

I see @josephsnow referenced his work to this issue, so he may be adding a pull request, otherwise it will need to wait a little until my work pressure decreases or somebody else do the pull request.

Regards,
Jon

@mgalindo
Copy link

@wvanrensburg I am creating my first loopback project and hitting the same issue you said to downgrading to 3.1.1 helped you as a workaround. May I ask what module did you downgrade?

@alemhnan
Copy link

loopback itself:

"loopback": "^3.1.1",

@mgalindo
Copy link

@alemhnan Thanks for your reply. I have tried 3.0.0, 3.1.1 and 3.2.1 with the same error as the OP.

@alemhnan
Copy link

I'm using "@mean-expert/loopback-sdk-builder": "^2.1.0-rc.8.2" for the sdk builder and loopback 3.1.1 without experiencing the problem.
Have you maybe tried to clean the node_modules directory?

@mgalindo
Copy link

@alemhnan I have tried deleting the node_modules folder and clearing the npm cache with no luck
This is what I am using
"loopback": "^3.1.1",
"loopback-boot": "^2.23.0",
"loopback-component-explorer": "^4.0.0",
"loopback-connector-mongodb": "^1.18.0",

"@mean-expert/loopback-sdk-builder": "^2.1.0-rc.8.2",

@JonnyBGod
Copy link
Contributor

Lock loopback version

"loopback": "^3.1.1" >>> "loopback": "3.1.1"

@mgalindo
Copy link

Thanks for the help everybody, that fixed my issue.

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.

9 participants