Best way to add "for current user" routes that don't take a User ID?

[Mr-Wallet]

My team is trying to move from the original node-gitlab repo to this one, primarily for v4 support. A few routes we need are still not implemented here, so we're interested in opening some pull requests.

One that is not totally straightforward is routes that sometimes take an ID and sometimes don't. For example, in UserKeys, all requires a userID so it can make a GET to /users/${id}/keys. But there's also a route /user/keys (docs) to make a request for the currently-authenticated user based on their token (without having to first ask the API what the user's ID is).

I was just wondering how best we could fit things of this pattern into this project. Since it's listed in the Users documentation, on our fork of node-gitlab we put the implementation into UserKeys and called it the somewhat-verbose allForCurrentUser. Does this work for you or is there something you think would fit better?

[jdalrymple]

Hey! I've recently tackled this problem and will have a beta release ready by the end of this weekend. for this case it will be somehting like

UserKeys.all({userId})

Where the userId is optional, if its passed it with be for that user, else it will be for the currently authenticated user.

Does this work for you?

[Mr-Wallet]

Honestly that makes even more sense than GitLab's own URI design. As long as it also has add and remove, that will actually be almost everything we need. While I have your attention, do you think you could sneak "get forks" into that release?

[jdalrymple]

Yup!

[jdalrymple]

Check the most recent beta release, tell me if it works for you!

[Mr-Wallet]

I was skimming the diff and I noticed the push to Node 9. Did you sneak any Node 9 features in? We have a big cruise ship of an app and only fairly recently managed to get it on Node 8, so that would be a no-go for us. If it's safe to do so, perhaps on our own fork of this we could downgrade that assertion back to 8, for the sake of our CI/linter.

[jdalrymple]

Yea a few, I can put it back to what it was before which compiled down to 8.9.0 ? Just trying to keep the the library up to date :(

[Mr-Wallet]

Besides the immediate inconvenience to us personally, I'm most concerned about us using something versioned against 9 because odd-numbered Node releases do not go into LTS when development finishes on them. For our part, I suspect we're probably going to stay on 8 until we can make the jump to 10.

Unfortunately you seem to have the only good node module for gitlab API v4 at the moment... 😅 We'd love to rely on this excellent package, but in our particular case we need to distinguish between "up to date" and "bleeding edge". If you don't expect to use anything exclusive to Node 9, it may be better for adoption to stick with 8. If that's unpalatable then we can fork this and build it against 8 ourselves - but if we can, we'd like to avoid having to worry about pulling in ongoing updates from this base repo.

[jdalrymple]

thats fair! Ill keep it compiling down to 8 lts!

[Mr-Wallet]

By the way, I really love the defaulting = {} behavior for options, so you might consider doing that for e.g. UserKeys, so that people don't need to provide an empty object when omitting user ID.

[jdalrymple]

Oops! Must of missed it thanks!

[jdalrymple]

Just pushed another release with some more fixes. Going to let it settle for a few days to see if i missed anything serious and then ill officially release it

[Mr-Wallet]

It's not clear what the intended way to use 3.0.0 is, aside from the addition of the new keyword. (We definitely want to stick with the "Basic Import" since we are going to use many namespaces.) I really like v2's way of not needing to use new, and getting an object where namespaces can be accessed as properties. I think the Examples section in rc2's README may be out of date; or if not, it should define what the api variable refers to.

[jdalrymple]

I agree with not using the new keyword, but in v2 it was inconsistent and without it would require alot of self initialization which was what i was trying to avoid. It is annoying though im aware :P.

Ill update the examples to show the import so the variable makes more sense. Its just a reference to the above basic import examples.

Minus the new key word, the availabilty of the namespaces as properties are still present!

import Gitlab from 'node-gitlab-api';

const api = new Gitlab({ my credentials here });

api.Projects.all()
api.Groups.all()
api.SystemHooks.show(2) 

//etc

Sorry for the confusion!! If i can make it any clearer please dont hesitate to say anything :D

Edit: Added more docs, tell me if that helps !!

[Mr-Wallet]

That's definitely what I was expecting. The issue I was having is that when I create a new Gitlab, I end up with something that doesn't have those properties on it. Instead it's something with the Namespace prototype.

[jdalrymple]

Ahh!! I misunderstood sorry. I'll look !

…

On Mar 20, 2018, 5:00 PM, at 5:00 PM, Jordan Wallet ***@***.***> wrote: The issue I was having is that when I create a `new Gitlab`, I end up with something that doesn't have those properties on it. Instead it's something with the `Namespace` prototype. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #44 (comment)

[Mr-Wallet]

I had assumed you intentionally changed something 😆

[jdalrymple]

😂 I did expose it as a namespace but it should have all the properties in it, so if it doesn't I must of broke something

…

On Mar 20, 2018, 5:14 PM, at 5:14 PM, Jordan Wallet ***@***.***> wrote: I had assumed you intentionally changed something 😆 -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #44 (comment)

[jdalrymple]

Yup, i had conflicting variable names. Fixing now!!

[jdalrymple]

Should be good to go now :) v3.0.0-rc.3