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

Use TypeScript #9

Closed
gioragutt opened this issue Sep 27, 2017 · 14 comments
Closed

Use TypeScript #9

gioragutt opened this issue Sep 27, 2017 · 14 comments

Comments

@gioragutt
Copy link
Collaborator

I think that re-writing the code in typescript could really help this library, for the following reasons -

  1. This will make it easy to integrate into typescript based codebases
  2. This will help with readability and maintenance of the code
  3. This will allow us to use the newest JS/TS features, while keeping compatibility with older Node versions

Think about the inRange pull-request - we could not use array destructuring because this messed up compatibility with Node v4.
If we write this in typescript, the compiled js will be compatible with older versions, while the source code will be written with every JS feature we see fit.

@sindresorhus
Copy link
Owner

Sure. I would like to see #4 merged first though.

@sindresorhus sindresorhus changed the title Refactor library to use typescript Use TypeScript Sep 28, 2017
@sindresorhus
Copy link
Owner

This can be implemented now.

@gioragutt
Copy link
Collaborator Author

gioragutt commented Sep 30, 2017

@sindresorhus
There are two ways to expose typescript.

  1. to re-write entirely with typescript (and have the .d.ts file an artifact of the compilation)
  2. to expose a manual .d.ts

If we mean to expose a manual .d.ts file, it would look something like this:

declare module "@sindresorhus/is" {
    export interface Is {
        (val: any): string;
        undefined: TypeQueryMethod;
        null: TypeQueryMethod;
        string: TypeQueryMethod;
        number: TypeQueryMethod;
        boolean: TypeQueryMethod;
        symbol: TypeQueryMethod;
    
        array: TypeQueryMethod;
        function: TypeQueryMethod;
        buffer: TypeQueryMethod;
        object: TypeQueryMethod;
    
        nativePromise: TypeQueryMethod;
        promise: TypeQueryMethod;
        regExp: TypeQueryMethod;
        date: TypeQueryMethod;
        error: TypeQueryMethod;
        map: TypeQueryMethod;
        set: TypeQueryMethod;
        weakMap: TypeQueryMethod;
        weakSet: TypeQueryMethod;
    
        int8Array: TypeQueryMethod;
        uint8Array: TypeQueryMethod;
        uint8ClampedArray: TypeQueryMethod;
        int16Array: TypeQueryMethod;
        uint16Array: TypeQueryMethod;
        int32Array: TypeQueryMethod;
        uint32Array: TypeQueryMethod;
        float32Array: TypeQueryMethod;
        float64Array: TypeQueryMethod;
        arrayBuffer: TypeQueryMethod;
        sharedArrayBuffer: TypeQueryMethod;
    
        nan: TypeQueryMethod;
        nullOrUndefined: TypeQueryMethod;
        primitive: TypeQueryMethod;
        integer: TypeQueryMethod;
        plainObject: TypeQueryMethod;
        iterable: TypeQueryMethod;
        class: TypeQueryMethod;
        typedArray: TypeQueryMethod;
        inRange: {
            (value: number, range: number): boolean;
            (value: number, [first, last]: Array<number>): boolean;
        }
    }

    const is: Is;
}

not exactly sure on how do I test it, but I'm sure there's an easy way. Gonna keep figuring it out.

If we doing the re-write in ts approach, I'm still figuring out how do we declare is. See my stackoverflow question.

@gioragutt
Copy link
Collaborator Author

@sindresorhus if we re-write in ts and run the tests on the output .js file, it will not pass the linter that you're using

@yeskunall
Copy link
Contributor

Just chiming in here: I don't really see the need to rewrite this in TS. TS works well with large codebases and I don't see the need to bring in the concept of static-typing in here. TS can act as a replacement for Babel, but once again, in this case, I don't see the need to.

A simple build script that transpiles ES6+ code should suffice. Again, this is just my input on this matter. What are you guys' thoughts on this @gioragutt @sindresorhus?

@sindresorhus
Copy link
Owner

not exactly sure on how do I test it, but I'm sure there's an easy way. Gonna keep figuring it out.

We can just make a TS file with various use-cases and run TS on it.

Just chiming in here: I don't really see the need to rewrite this in TS.

Yeah, I think TS type definitions would be good enough for our needs.

A simple build script that transpiles ES6+ code should suffice.

I don't really see the point of that.

@yeskunall
Copy link
Contributor

yeskunall commented Oct 2, 2017

I guess it's decided. I'm not at all familiar with TS, so if you see the benefit, then there's nothing more for me to add here. 🚶

Think about the inRange pull-request - we could not use array destructuring because this messed up compatibility with Node v4.

^ transpiling would help in this case, wouldn't it? Just wondering 🤔

@sindresorhus
Copy link
Owner

^ transpiling would help in this case, wouldn't it? Just wondering 🤔

Not worth adding a build-step just for a tiny syntactic feature.

@sindresorhus
Copy link
Owner

@gioragutt Thoughts? Could you do a PR with your above type definition?

@gioragutt
Copy link
Collaborator Author

@sindresorhus I will when I'll have some free time for it, currently rather busy.

I do have to notice #8, I'm currently using CRA, and I'd have loved to have it, but as you can see, the issue submitter says that CRA can't deal with it. I'd say this is slightly higher priority than TS support.

@gioragutt
Copy link
Collaborator Author

@sindresorhus I do have to say now, I've tried installing is in my project, it seems to work correctly, but we're do missing type definitions for IDE completions.

@gioragutt
Copy link
Collaborator Author

gioragutt commented Oct 6, 2017

Not worth adding a build-step just for a tiny syntactic feature.

@sindresorhus we could really use some ES5 features, I do believe that it's worth adding a step (which can be automatically done via npm scripts) to allow us to write much cleaner code, while keeping the compatibility that we want.

Consider that. Take f.e #19 , he ends up having to use arguments because (pred, ...values) => {...} won't work on node4.

The more we delay this, the more we have to write less readable and maintainable code, and to keep it Well maintained, it has to be as maintainable as we can bring it to be.

I know you have dozens of small, 10 liner libraries, but this isn't one.

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 7, 2017

Having to use older syntax creates overhead, true, but so does a build step. I wouldn't add a build step personally, but I'm ok with doing it here if someone else does the work. But if we're going to add a build step, it would make more sense to do the module in TypeScript. That way we don't have to manually keep the type definition file up to date.

@sindresorhus
Copy link
Owner

Fixed by #28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants