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 types declaration file #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geopic
Copy link

@geopic geopic commented Mar 7, 2020

Resolves #20. Let me know if there are any issues.

@dirkesquire
Copy link

Yes please! I need this for it to work in Typescript. Are there any holdups to prevent this pull request?

@velipso
Copy link
Owner

velipso commented Jun 2, 2020

Yes, sorry for the delay -- the types are probably good enough for the public API, but the details are wrong.

For more accurate types, I would like segments and combined be separate opaque types.

@velipso
Copy link
Owner

velipso commented Jun 2, 2020

The Flavor strategy in this blog post might be a good way to do opaque types in TS:

https://spin.atomicobject.com/2018/01/15/typescript-flexible-nominal-typing/

Something like:

type Segments = { _type?: 'PolyboolJsSegments' };
type CombinedSegments = { _type?: 'PolyboolJsCombinedSegments' };

@dirkesquire
Copy link

I'm honestly struggling to get this project working with Typescript at all. Are there any other options I can use in the meantime?

@geopic
Copy link
Author

geopic commented Jun 6, 2020

Unfortunately what @voidqk has suggested is outside of my knowledge at the moment. What I've submitted works according to my own tests but may not work (or only work partially) elsewhere. So I can't be of much more help than this. 🤷‍♂️

@dirkesquire
Copy link

dirkesquire commented Jun 6, 2020

Thanks geopic. So it does work for you?
I'm just not smart enough to figure out why this is not working. What I've done is add polybooljs to my project and then to manually create a file polybooljs/index.d.ts and placed the contents of your file from the pull request in there. But I get these errors:

ERROR in [at-loader] ./src/start.ts:125:1 
    TS2693: 'PolyBool' only refers to a type, but is being used as a value here.

ERROR in [at-loader] ./src/start.ts:126:7 
    TS2339: Property 'union' does not exist on type 'typeof import("polybooljs")'.

This is my source code. I just can't seem to call the union function no matter what I try:

import PolyBool from 'polybooljs'
import * as PolyBool2 from 'polybooljs'
import { Polygon, Segment, GeoJSON } from 'polybooljs'

let poly1: Polygon = {
    regions:  [[ [ 1, 1 ], [ 1, 2 ], [ 2, 2 ], [ 2, 1 ] ]],
    inverted: false
}

let poly2: Polygon = {
    regions: [[ [ 1, 1 ], [ 1, 2 ], [ 2, 2 ], [ 2, 1 ] ]],
    inverted: false
}

PolyBool.union(poly1, poly2)         // Gives first error
PolyBool2.union(poly1, poly2)      // Gives second error

@geopic
Copy link
Author

geopic commented Jun 6, 2020

Ok, on second thought, I'll look at it again. I think I know what's up. My IDE sometimes complains about these things then sometimes doesn't (when it should). I'll give it another look and will submit a squashed commit on Monday unless I cannot make it work, in which case I'll let you guys know. Thanks for the heads up @dirkesquire.

@geopic geopic force-pushed the types-declaration-file branch from d5c72f0 to 24ebe41 Compare June 8, 2020 04:48
@geopic
Copy link
Author

geopic commented Jun 8, 2020

Ok, I've rebased onto the previous commit so it should still be one commit being added. There were some issues with the file I wrote back in March which I've fixed and now it works 100% with the TypeScript compiler. The types themselves are my best guess but at least the types file is structured properly now. I believe you shouldn't have any more issues @dirkesquire but let me know if you do. 👍

@dirkesquire
Copy link

Thanks, this works for me!

@pahuta
Copy link

pahuta commented Dec 7, 2020

Guys, let's merge this PR. I need it in production 🙂.

@alburdette619
Copy link

Merge it y'all!

@geopic
Copy link
Author

geopic commented Feb 22, 2021

Paging @velipso

@velipso
Copy link
Owner

velipso commented Feb 22, 2021

The types aren't correct... I haven't looked at this code in a while, but Segments is wrong, and combine is currently returning combined: object, which is also wrong...

I realize that you don't need these types to be accurate for regular usage, but I'm not going to merge it if the types are wrong. I also don't understand the urgency of merging the types... you can drop your own types into TypeScript using .d.ts files. TypeScript is specifically designed to allow third-party types. If this file is good enough for your usage, grab it, and drop it into your build.

Also, feel free to fork and land the types if you really want to. I don't look at this repo often.

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.

TypeScript type declaration file
5 participants