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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to latest @octokit/rest #983

Merged
merged 4 commits into from
Feb 3, 2020
Merged

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented Feb 3, 2020

Hey friends 馃憢

In preparation of the upcoming @octokit/rest v17, I'm testing against some dependents such as Danger. While at it I hope to be helpful with the upgrade.

I already found one problem that I'm addressing here https://github.com/octokit/rest.js/pull/1586

Besides that, I get errors when building:

node_modules/memfs-or-file-map-to-github-branch/build/index.d.ts:30:50 - error TS2709: Cannot use namespace 'GitHub' as a type.

30 export declare const memFSToGitHubCommits: (api: GitHub, volume: MemFSVolume, settings: BranchCreationConfig) => Promise<void>;
                                                    ~~~~~~

I guess the problem is the change I did in @octokit/rest's index.d.ts?

TypeScript is still confusing the hell out of me. Got any pointers? Ideally I'd like to continue supporting both: import Octokit from "@octokit/rest" and import { Octokit } from "@octokit/rest" for TypeScript users. Starting with v17, it will only be import { Octokit } from "@octokit/rest", so when in doubt, I don't mind breaking the import Octokit from "@octokit/rest" usage for TypeScript only. Either works in JS in the latest v16 versions

Note that this PR does relies on the change in https://github.com/octokit/rest.js/pull/1586 in order to reproduce the TypeScript build error.

@orta
Copy link
Member

orta commented Feb 3, 2020

Yeah, that error is because I'm doing this here - I can fix that to import { Octokit as GitHub } from "@octokit/rest" 馃憤

To support all forms of the export, you probably want something like:

export class OctoKit {}

export default OctoKit

Getting the syntax without the * as may still require esModuleInterop being turned on

@gr2m
Copy link
Contributor Author

gr2m commented Feb 3, 2020

Yeah, that error is because I'm doing this here - I can fix that to import { Octokit as GitHub } from "@octokit/rest" 馃憤

I was just looking into that, PR incoming

To support all forms of the export, you probably want something like:

export class OctoKit {}

export default OctoKit

I tried that, but when doing

import * as OctokitDeprecated from "../index";
const { Octokit } = OctokitDeprecated

// Check constructor
new Octokit();

// Check deprecated constructor
new OctokitDeprecated();

I see this error

This expression is not constructable.
Type 'typeof import("/Users/gregor/Projects/octokit/rest.js/index")' has no construct signatures.ts(2351)

@fbartho
Copy link
Member

fbartho commented Feb 3, 2020

Yeah, my experience is that default exports explode often under TypeScript :-/

@gr2m
Copy link
Contributor Author

gr2m commented Feb 3, 2020

@orta can you publish a new version of memfs-or-file-map-to-github-branch? Currently yarn build fails

yarn build

yarn run v1.21.1
$ shx rm -rf ./distribution && tsc -p tsconfig.production.json && madge ./distribution --circular
node_modules/memfs-or-file-map-to-github-branch/build/index.d.ts:30:50 - error TS2709: Cannot use namespace 'GitHub' as a type.

30 export declare const memFSToGitHubCommits: (api: GitHub, volume: MemFSVolume, settings: BranchCreationConfig) => Promise<void>;
                                                    ~~~~~~

node_modules/memfs-or-file-map-to-github-branch/build/index.d.ts:34:69 - error TS2709: Cannot use namespace 'GitHub' as a type.

34 export declare const filepathContentsMapToUpdateGitHubBranch: (api: GitHub, fileMap: FileMap, settings: BranchCreationConfig) => Promise<void>;
                                                                       ~~~~~~

node_modules/memfs-or-file-map-to-github-branch/build/index.d.ts:43:40 - error TS2709: Cannot use namespace 'GitHub' as a type.

43 export declare const createTree: (api: GitHub, settings: BranchCreationConfig) => (fileMap: FileMap, baseSha: string) => Promise<any>;
                                          ~~~~~~

node_modules/memfs-or-file-map-to-github-branch/build/index.d.ts:49:43 - error TS2709: Cannot use namespace 'GitHub' as a type.

49 export declare const createACommit: (api: GitHub, settings: BranchCreationConfig) => (treeSha: string, parentSha: string) => Promise<any>;
                                             ~~~~~~

node_modules/memfs-or-file-map-to-github-branch/build/index.d.ts:57:45 - error TS2709: Cannot use namespace 'GitHub' as a type.

57 export declare const updateReference: (api: GitHub, settings: BranchCreationConfig) => (newSha: string) => Promise<any>;
                                               ~~~~~~

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@orta
Copy link
Member

orta commented Feb 3, 2020

Shipped - I don't have an answer about the default issue there.

Does seem weird to me though.

( I also killed all cache on travis and restarted the CI )

@gr2m gr2m changed the title 馃毀 Upgrade to latest @octokit/rest Upgrade to latest @octokit/rest Feb 3, 2020
@gr2m
Copy link
Contributor Author

gr2m commented Feb 3, 2020

I think we are all set

@orta
Copy link
Member

orta commented Feb 3, 2020

Yep, this works for me - thanks! Good luck with the release

@orta orta merged commit 0c5b23f into danger:master Feb 3, 2020
@gr2m gr2m deleted the upgrade-octokit-rest branch February 3, 2020 23:00
@gr2m gr2m mentioned this pull request Feb 19, 2020
28 tasks
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

3 participants