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

Add support for --isolatedModules #53

Merged
merged 4 commits into from May 22, 2020

Conversation

mishkinf
Copy link
Contributor

@mishkinf mishkinf commented Mar 19, 2020

This library doesn't compile in typescript projects that use isolatedModules=true in their tsConfig.js such as a Create React App. The reason being is because types are shared across several of the defined packages. Sharing the types prevents the compiler from isolating the modules.

This PR addresses that issue by importing the types and re-exporting them by using typescript's new type only import / exports and re-exports types so modules can be isolated.

Refer to https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html

Unfortunately Prettier does not support typescript 3.8 yet so I have disabled prettier in this PR for now as well. Refer to prettier/prettier#7263

The error looks like:


/Users/blah/workspace/project/node_modules/bitbucket/src/client/types.ts
TypeScript error in /Users/blah/workspace/project/node_modules/bitbucket/src/client/types.ts(6,10):
Re-exporting a type when the '--isolatedModules' flag is provided requires using 'export type'.  TS1205

    4 | import { Request, Response } from '../request/types'
    5 |
  > 6 | export { EndpointParams } from '../endpoint/types'
      |          ^
    7 |
    8 | export interface Options {
    9 |   [option: string]: any

@MunifTanjim
Copy link
Owner

MunifTanjim commented Mar 21, 2020

I don't think jumping to TypeScript 3.8 would be a good idea. 🤔
Many people are still stuck with older TypeScript versions.

@mishkinf
Copy link
Contributor Author

Sure. I just can’t use this repo without the change. I may have to cut a separate release for my own needs.

Another solution would be to change how project is broken into modules but it was more involved than I wanted to go for my own needs.

@MunifTanjim
Copy link
Owner

Hey @mishkinf,

Sorry for the late reply.

Can you please change all of the type imports & exports like this in the types.ts files?

// this is just imported (can be used in this file)
type APIClient = import('../../client/types').APIClient

// this is both imported (can be used in this file) & exported (can be imported from another file)
export type APIClient = import('../../client/types').APIClient

This should solve the problem with isolatedModules=true and we won't need TS 3.8.

@PetrShchukin
Copy link

When the pull request is gonna be merged? Have the same problem with typescript

@mishkinf
Copy link
Contributor Author

mishkinf commented May 18, 2020

Based on what MunifTanjim has said as the owner, which makes sense, it is unlikely to get merged as it is now. However there is a need for somebody to restructure this project in a typescript friendly way the way it described by him in his comment on March 25. If you or anybody would like to take that on please do! You can also fork my branch and publish your own package if you'd like..

@PetrShchukin
Copy link

So far I can't even make it work with React.. As I need some little piece of functionality I think I will go with bitbucket rest api

@MunifTanjim MunifTanjim merged commit c54370c into MunifTanjim:master May 22, 2020
@MunifTanjim
Copy link
Owner

Thanks @mishkinf for the initial PR. I've refactored and merged it.

@PetrShchukin this will be published shortly.

This also resolves #61

MunifTanjim added a commit that referenced this pull request May 22, 2020
@mishkinf mishkinf deleted the isolated-modules branch May 23, 2020 17:58
@mishkinf
Copy link
Contributor Author

thank you @MunifTanjim ! all those typescript / create react app people will be happy out there. :)

@PetrShchukin
Copy link

Cheers!

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