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

Version 5.0.0 #216

Merged
merged 26 commits into from
May 25, 2019
Merged

Version 5.0.0 #216

merged 26 commits into from
May 25, 2019

Conversation

jdalrymple
Copy link
Owner

@jdalrymple jdalrymple commented Oct 12, 2018

Summary

Breaking

  • Added content as a required parameter for RepositoryFiles
  • Removed projectId from System Hooks API since it wasn't required
  • Removed dependency on FS. Now the Projects API takes in two arguments projectId and content as well as an option fileName argument
  • Changing everything to named exports for simplicity
  • Switching required initialization argument from 'url' to 'host'
  • Updated Approvals API support to match https://docs.gitlab.com/ee/api/merge_request_approvals.html
  • MergeRequest Pipelines require the mergeRequestId
  • NotificationSettings API edit function now takes one parameter, options
  • Changing the access level enum property from master to maintainer as per https://gitlab.com/gitlab-org/gitlab-ce/issues/42751
  • Notes now require a body argument instead of checking the options argument for a body parameter

Bug Fixes

Features

  • Added the ability to add sudo to specific requests (780244f (780244f))
  • Added the missing edit function to the Groups API
  • Added LDAP support to the Groups API

Documentation

  • Removing xml request docs
  • Updating imports to be named imports

@jdalrymple jdalrymple added this to the 5.0.0 milestone Oct 12, 2018
@jdalrymple jdalrymple changed the title Next major release Version 5.0.0 Nov 11, 2018
@jdalrymple
Copy link
Owner Author

Releasing Rc.1 soon!

@jetersen
Copy link
Contributor

lots of conflicts, jikes 😱

@jdalrymple
Copy link
Owner Author

@Casz Yea, going to do my best to rebase the branch

@jetersen
Copy link
Contributor

jetersen commented Nov 11, 2018

@jdalrymple
Copy link
Owner Author

lol i definitely messed up some things when rebasing, working on fixing them up now!

src/types.d.ts Outdated Show resolved Hide resolved
@jdalrymple
Copy link
Owner Author

....it passed :o, time to clean up the git history

@jdalrymple
Copy link
Owner Author

@Casz Do you mind giving it a look over? I'm sure i missed things. If all goes well, ill push to a rc build on npm within the next day.

@jdalrymple
Copy link
Owner Author

Published under the next tag

@geektheripper
Copy link

geektheripper commented Nov 16, 2018

Awesome,but it seems that you forgot publish dist/index.d.ts
image
@jdalrymple

@jdalrymple
Copy link
Owner Author

Oops! Ill fix that now!

@jdalrymple
Copy link
Owner Author

Should be published now. Not sure if i should have the index.d.ts file in the root or in the dist folder since im not generating it...

@geektheripper
Copy link

I read some some source code, I think it's not a good idea to use build a bundle for nodejs by rollup, and you should publish declaration files to help typescript users.

The types in package.json is used to target the generated declaration file, not the types you used as global type defination in your project. Readmore

In my view, it's better to remove node build in rollup config, and use tsc to build the node version.

then project will looks like following tree:

├── dist
│   ├── index.browser.js
│   ├── index.d.ts
│   ├── index.es.js
│   ├── index.js
│   ├── infrastructure
│   │   ├── *.js
│   │   └── *.d.ts
│   └── ...
├── package.json
├── rollup.config.js
├── src
│   ├── index.ts
│   ├── infrastructure
│   ├── ...
│   └── types
│       └── index.d.ts
└── ...

BTW, there's some issue in you types file:

https://github.com/jdalrymple/node-gitlab/blob/78108913b7c71ba88f6c81c3249bdcdfcd3be185/index.d.ts#L79

https://github.com/jdalrymple/node-gitlab/blob/78108913b7c71ba88f6c81c3249bdcdfcd3be185/index.d.ts#L112-L118

@jdalrymple
Copy link
Owner Author

I can easily turn on the declaration file, but only three things get generated. It isnt that helpful lol.

@jdalrymple
Copy link
Owner Author

Im a little confused, and my research hasnt been very fruitful. Should the generated declaration file (index.d.ts) include all the types ive predefined in the project? And if so, how does one setup up the configuration to do so?

@jdalrymple
Copy link
Owner Author

@Mr-Wallet Do you see any problems that could arise in terms of the xhr compatibilities ?

@Mr-Wallet
Copy link
Contributor

According to ky's docs, it uses the Request API instead of XHR, so that's a no-go.

Skimming the changes (and I emphasize skimming) I don't immediately see any way we could inject XHR ourselves. So we (and potentially other electron apps) are going to be locked out of this version.

I appreciate the esoteric nature of our XHR requirement so I don't mind if all mention of it is scrubbed from the project, but we at least need a way to write a wrapper for XHR that we can plug into this project to make requests, or else we're hosed.

@jdalrymple
Copy link
Owner Author

Thank you for looking! If thats the case, well have to find another way around it :P Alot of the changes were made to ensure cross compatibility. I asked the question in the ky repo to ensure that conclusion was correct before investigating further.

sindresorhus/ky#62

@Mr-Wallet
Copy link
Contributor

Mr-Wallet commented Nov 21, 2018

They closed the issue stating it's not supported, and based on the project's apparent goals, I think it would be strange to expect them to do so. I feel that the onus is on node-gitlab to be pluggable. I'd like to stress that I'm fine if node-gitlab doesn't support XHR directly, as long as it supports dependency injection where we can provide our own requester which wraps XHR. This basically just means putting one abstraction layer between the requests and got/ky (which I think you already pretty much have, in the form of Request Helper) and then allowing users to provide that implementation as a parameter.

@jdalrymple
Copy link
Owner Author

😂 I was really hoping on removing that complexity, but i see that really isnt possible. Ill add back that functionality asap!

@sebald
Copy link

sebald commented Nov 26, 2018

@jdalrymple @geektheripper The problem is the src/infrastructure/Utils.ts/bundler nullifies all type inference. It produces typings like:

export declare const Gitlab: {
    new (options?: any): {
        [serviceName: string]: import("./infrastructure/BaseService").default;
    };
};

In order to get proper typings something like this should be used:

export interface Constructor {
  new (...args: any): any;
}
export type Mapper<T extends { [name: string]: Constructor }, P extends keyof T> = {
  [name in P]: InstanceType<T[P]>
};

export const bundler = <T extends { [name: string]: Constructor }, P extends keyof T>(
  services: T,
) => (options?: any) =>
  Object.entries(services || {}).reduce((bundle, [name, ser]) => {
    bundle[name] = new ser(options);
    return bundle;
  }, {}) as Mapper<T, P>;

I had to not use a class since computed properties are not really supported when using classes :-/

While the above makes the typings work, it changes the API (no classes, new).

screenshot 2018-11-26 at 12 03 06 1


Here are the produced typings (npm run build):

import * as APIServices from './services';
export * from './services';
export declare const GroupsBundle: (options?: any) => import("./infrastructure/Utils").Mapper<{
    Groups: typeof APIServices.Groups;
    GroupAccessRequests: typeof APIServices.GroupAccessRequests;
    GroupBadges: typeof APIServices.GroupBadges;
    GroupCustomAttributes: typeof APIServices.GroupCustomAttributes;
    GroupIssueBoards: typeof APIServices.GroupIssueBoards;
    GroupMembers: typeof APIServices.GroupMembers;
    GroupMilestones: typeof APIServices.GroupMilestones;
    GroupProjects: typeof APIServices.GroupProjects;
    GroupVariables: typeof APIServices.GroupVariables;
    Epics: typeof APIServices.Epics;
    EpicIssues: typeof APIServices.EpicIssues;
    EpicNotes: typeof APIServices.EpicNotes;
    EpicDiscussions: typeof APIServices.EpicDiscussions;
}, "Groups" | "GroupAccessRequests" | "GroupBadges" | "GroupCustomAttributes" | "GroupIssueBoards" | "GroupMembers" | "GroupMilestones" | "GroupProjects" | "GroupVariables" | "Epics" | "EpicIssues" | "EpicNotes" | "EpicDiscussions">;
export declare const UsersBundle: (options?: any) => import("./infrastructure/Utils").Mapper<{
    Users: typeof APIServices.Users;
    UserCustomAttributes: typeof APIServices.UserCustomAttributes;
    UserEmails: typeof APIServices.UserEmails;
    UserImpersonationTokens: typeof APIServices.UserImpersonationTokens;
    UserKeys: typeof APIServices.UserKeys;
    UserGPGKeys: typeof APIServices.UserGPGKeys;
}, "Users" | "UserCustomAttributes" | "UserEmails" | "UserImpersonationTokens" | "UserKeys" | "UserGPGKeys">;
export declare const ProjectsBundle: (options?: any) => import("./infrastructure/Utils").Mapper<{
    Branches: typeof APIServices.Branches;
    Commits: typeof APIServices.Commits;
    CommitDiscussions: typeof APIServices.CommitDiscussions;
    DeployKeys: typeof APIServices.DeployKeys;
    Deployments: typeof APIServices.Deployments;
    Environments: typeof APIServices.Environments;
    Issues: typeof APIServices.Issues;
    IssueAwardEmojis: typeof APIServices.IssueAwardEmojis;
    IssueNotes: typeof APIServices.IssueNotes;
    IssueDiscussions: typeof APIServices.IssueDiscussions;
    Jobs: typeof APIServices.Jobs;
    Labels: typeof APIServices.Labels;
    MergeRequests: typeof APIServices.MergeRequests;
    MergeRequestAwardEmojis: typeof APIServices.MergeRequestAwardEmojis;
    MergeRequestDiscussions: typeof APIServices.MergeRequestDiscussions;
    MergeRequestNotes: typeof APIServices.MergeRequestNotes;
    Pipelines: typeof APIServices.Pipelines;
    PipelineSchedules: typeof APIServices.PipelineSchedules;
    PipelineScheduleVariables: typeof APIServices.PipelineScheduleVariables;
    Projects: typeof APIServices.Projects;
    ProjectAccessRequests: typeof APIServices.ProjectAccessRequests;
    ProjectBadges: typeof APIServices.ProjectBadges;
    ProjectCustomAttributes: typeof APIServices.ProjectCustomAttributes;
    ProjectImportExport: typeof APIServices.ProjectImportExport;
    ProjectIssueBoards: typeof APIServices.ProjectIssueBoards;
    ProjectHooks: typeof APIServices.ProjectHooks;
    ProjectMembers: typeof APIServices.ProjectMembers;
    ProjectMilestones: typeof APIServices.ProjectMilestones;
    ProjectSnippets: typeof APIServices.ProjectSnippets;
    ProjectSnippetNotes: typeof APIServices.ProjectSnippetNotes;
    ProjectSnippetDiscussions: typeof APIServices.ProjectSnippetDiscussions;
    ProjectSnippetAwardEmojis: typeof APIServices.ProjectSnippetAwardEmojis;
    ProtectedBranches: typeof APIServices.ProtectedBranches;
    ProjectVariables: typeof APIServices.ProjectVariables;
    Repositories: typeof APIServices.Repositories;
    RepositoryFiles: typeof APIServices.RepositoryFiles;
    Runners: typeof APIServices.Runners;
    Services: typeof APIServices.Services;
    Tags: typeof APIServices.Tags;
    Triggers: typeof APIServices.Triggers;
}, "Branches" | "Commits" | "CommitDiscussions" | "DeployKeys" | "Deployments" | "Environments" | "Issues" | "IssueAwardEmojis" | "IssueNotes" | "IssueDiscussions" | "Jobs" | "Labels" | "MergeRequests" | "MergeRequestAwardEmojis" | "MergeRequestDiscussions" | "MergeRequestNotes" | "Pipelines" | "PipelineSchedules" | "PipelineScheduleVariables" | "Projects" | "ProjectAccessRequests" | "ProjectBadges" | "ProjectCustomAttributes" | "ProjectImportExport" | "ProjectIssueBoards" | "ProjectHooks" | "ProjectMembers" | "ProjectMilestones" | "ProjectSnippets" | "ProjectSnippetNotes" | "ProjectSnippetDiscussions" | "ProjectSnippetAwardEmojis" | "ProtectedBranches" | "ProjectVariables" | "Repositories" | "RepositoryFiles" | "Runners" | "Services" | "Tags" | "Triggers">;
export declare const Gitlab: (options?: any) => import("./infrastructure/Utils").Mapper<typeof APIServices, "Groups" | "GroupAccessRequests" | "GroupBadges" | "GroupCustomAttributes" | "GroupIssueBoards" | "GroupMembers" | "GroupMilestones" | "GroupProjects" | "GroupVariables" | "Epics" | "EpicIssues" | "EpicNotes" | "EpicDiscussions" | "Users" | "UserCustomAttributes" | "UserEmails" | "UserImpersonationTokens" | "UserKeys" | "UserGPGKeys" | "Branches" | "Commits" | "CommitDiscussions" | "DeployKeys" | "Deployments" | "Environments" | "Issues" | "IssueAwardEmojis" | "IssueNotes" | "IssueDiscussions" | "Jobs" | "Labels" | "MergeRequests" | "MergeRequestAwardEmojis" | "MergeRequestDiscussions" | "MergeRequestNotes" | "Pipelines" | "PipelineSchedules" | "PipelineScheduleVariables" | "Projects" | "ProjectAccessRequests" | "ProjectBadges" | "ProjectCustomAttributes" | "ProjectImportExport" | "ProjectIssueBoards" | "ProjectHooks" | "ProjectMembers" | "ProjectMilestones" | "ProjectSnippets" | "ProjectSnippetNotes" | "ProjectSnippetDiscussions" | "ProjectSnippetAwardEmojis" | "ProtectedBranches" | "ProjectVariables" | "Repositories" | "RepositoryFiles" | "Runners" | "Services" | "Tags" | "Triggers" | "Todos" | "PushRule" | "ApplicationSettings" | "BroadcastMessages" | "Events" | "FeatureFlags" | "GeoNodes" | "GitignoreTemplates" | "GitLabCIYMLTemplates" | "Keys" | "Licence" | "LicenceTemplates" | "Lint" | "Namespaces" | "NotificationSettings" | "Markdown" | "PagesDomains" | "Search" | "SidekiqMetrics" | "Snippets" | "SystemHooks" | "Version" | "Wikis">;

TypeScript can not produce flat typings (yet) so the dist folder will have subfolders with the .d.ts in it.


PS: Of course it would be nice to replace (options: any) with (options: BaseServiceOptions & {[name:string]: any}) to have at least some basic intellisense ;)

@sebald
Copy link

sebald commented Nov 26, 2018

Also the @typings can not be resolved in projects that use node-gitlab :-/

@jdalrymple
Copy link
Owner Author

jdalrymple commented Nov 26, 2018

@sebald Thanks! Ill look into fixing these up some more this week. I definitely still learning how to use these typing correctly 😆

@Mr-Wallet
Copy link
Contributor

Why would a user ever want to make sure that this package hid the pagination info? Wouldn't it help discoverability of the pagination system to always include it?

@jdalrymple
Copy link
Owner Author

jdalrymple commented May 21, 2019

It's an extra level of complexity. For example without that option, you get back an array of the data or the object itself. With this option is deeper in the object tree, which is annoying

Also if we were always returning all results ( like now) the pagination info was less important

@Mr-Wallet
Copy link
Contributor

Good point. I guess this is a more subjective design decision of how much you want to abstract it. I would say, it would be better to abstract it completely and fetch all pages, than to only fetch one page and hide from the programmer that there may be more pages unless they read the docs on this package very carefully.

I would prefer to always nest the response and put the pagination info front and center, because any abstraction over pagination is destined to be a leaky one, and my personal opinion is that a very leaky abstraction is more dangerous than no abstraction; but if it's a given that you're going to hide pagination info by default, then it's better to go ahead and fetch all of the pages before resolving.

@jetersen
Copy link
Contributor

@jdalrymple hint you can turn off the LGTM comments 😆

@jdalrymple
Copy link
Owner Author

Anything else stand out as an impediment to officially releasing?

@jetersen
Copy link
Contributor

Oh, it looks like you managed to do two LGTM integrations 😆

Repository owner deleted a comment from lgtm-com bot May 25, 2019
@jdalrymple
Copy link
Owner Author

The github app is supposed to replace the web application :P

@jdalrymple
Copy link
Owner Author

The tests keep failing intermittently because the gitlab instance isnt fully started in the allocated time -_-

@jdalrymple
Copy link
Owner Author

@Casz Do you think were good for an official release?

@jetersen
Copy link
Contributor

I think so, I haven't had a chance to test in our bot 😅
Too many dependencies updated and haven't had the time to maintain the bot 😰

@jdalrymple jdalrymple merged commit aa31353 into master May 25, 2019
@jdalrymple
Copy link
Owner Author

jdalrymple commented May 25, 2019

🎉 This PR is included in version 5.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.