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

TS definitions broken since the initialSettings change. #224

Closed
floriancargoet opened this issue Jul 27, 2020 · 16 comments
Closed

TS definitions broken since the initialSettings change. #224

floriancargoet opened this issue Jul 27, 2020 · 16 comments

Comments

@floriancargoet
Copy link

The recent initialSettings change has broken the TypeScript definitions.

Who is maintaining these type files?

@skratchdot
Copy link
Owner

Oh no! Sorry about that (I didn't realize there were typings). I considered rewriting v6 in ts, but thought it was overkill (or "too big" of a change).

I can maintain typings in this lib if it helps

@skratchdot
Copy link
Owner

skratchdot commented Jul 27, 2020

New typings should probably look like:

// Type definitions for react-bootstrap-daterangepicker
// Project: https://github.com/skratchdot/react-bootstrap-daterangepicker
// Definitions by: Ian Ker-Seymer <https://github.com/ianks>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.8

/// <reference types="react" />
/// <reference types="daterangepicker" />

declare namespace ReactBootstrapDaterangepicker {

    export interface EventHandler { (event?: any, picker?: any): any; }
    export interface CallbackHandler { (start?: any, end?: any, label?: any): any; }

    export interface Props {
        initialSettings?: daterangepicker.Options;

        // events supported by the upstream lib
        onApply?: EventHandler;
        onCancel?: EventHandler;
        onHide?: EventHandler;
        onHideCalendar?: EventHandler;
        onShow?: EventHandler;
        onShowCalendar?: EventHandler;

        // custom events in this lib
        onEvent?: EventHandler;
        onCallback?: CallbackHandler;
    }

    export class DateRangePicker extends React.Component<Props> { }
}

declare var DateRangePicker: typeof ReactBootstrapDaterangepicker.DateRangePicker;

declare module "react-bootstrap-daterangepicker" {
    export = DateRangePicker;
}

EDIT: Updating the definition above to incorporate the feedback in a comment below: #224 (comment)

I wonder if you can see if these work?

I can either figure out how to maintain in this lib or submit a PR to the one you linked.

Would be nice to keep a better initialSettings definition, but I don't have the time to write that atm :)

@floriancargoet
Copy link
Author

The extends daterangepicker.Options is incorrect and is what you want for initialSettings.
It refers to the wrapped daterangepicker options that is to say initialSettings.

I've never written a library type file but I think it should read:

export interface Props {
    initialSettings?: daterangepicker.Options;
    // ...

@skratchdot
Copy link
Owner

skratchdot commented Jul 27, 2020

Good call. I totally skipped reading this line:

export interface Props extends daterangepicker.Options

I misread it as extending React.Props. Your suggestion looks correct to me.

I updated my comment/definition above to incorporate your feedback

@floriancargoet
Copy link
Author

I've tested your edited typings, they work.
EventHandler could be better:

...
/// <reference types="jquery" />

declare namespace ReactBootstrapDaterangepicker {
  export interface EventHandler {
    (event: JQuery.Event, picker: daterangepicker): any;
  }
  ...

I haven't used CallbackHandler yet so I haven't checked the exact types.

@skratchdot
Copy link
Owner

Here is what's passed to the callback:
https://github.com/dangrossman/daterangepicker/blob/6075b419d6072010accda52080322e1487c4cc7b/daterangepicker.js#L1160

The documentation says start/end are Date or String but I'm assuming that's really: Date or Moment or String 🤷 . I think the label is probably always a String, but not 100% sure about that. Thanks for your updated EventHandler !!!

@skratchdot
Copy link
Owner

Seems like I should keep this in this repo:
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#including-declarations-in-your-npm-package

Then potentially someday:
https://github.com/DefinitelyTyped/DefinitelyTyped#removing-a-package

@JeremyCade
Copy link

Is there a PR with the updated typings anywhere?

@skratchdot
Copy link
Owner

@JeremyCade - I will include the typings in this repo and publish a new package. I'm not sure about semver though: is this major/minor/patch? I might just do a patch release since it doesn't appear to be working for people w/ typescript.

@skratchdot
Copy link
Owner

Actually- I'm looking at my build, and I'm gonna have to change the rollup script (or add a new copy script). I think I'm just gonna re-write the source in TS and use the generated typings. I'll publish a new major version afterwards. I'll try to get to this today if I have enough time

@JeremyCade
Copy link

Thanks @skratchdot. I'm sure you'll get to you as/when you have time.

@skratchdot
Copy link
Owner

I just pushed a change for this and published a beta version.

@floriancargoet / @JeremyCade - Can you try running this in your repo:

npm uninstall @types/react-bootstrap-daterangepicker
npm install react-bootstrap-daterangepicker@7.0.0-beta.0

and see if things work for you?

If it does, I will publish v7.0.0. Thanks!

@floriancargoet
Copy link
Author

TypeScript correctly loads your new file but it has the old definitions.

export interface Props extends daterangepicker.Options {
    initialSettings?: any;

Also, $picker is declared as any, which could be better.

     $picker: JQuery;

@skratchdot
Copy link
Owner

Oh weird. They are "auto-generated" so I think I copied the wrong version into the src/index.tsx file. I will fix, thanks!

@skratchdot
Copy link
Owner

Now I remember: I tried $picker: JQuery and it raised a bunch of issues I couldn't figure out, but I definitely missed the initialSettings?: daterangepicker.Options change.

Anyways, I published commit 7c94185 as:
react-bootstrap-daterangepicker@7.0.0-beta.1

Can you try again?

I'm not sure how to "fix" $picker: JQuery;, but if you wanna submit a PR, I can test/merge it.

You should be able to edit:
https://github.com/skratchdot/react-bootstrap-daterangepicker/blob/master/src/index.tsx#L34

Then try to figure out how to fix all the issues. npm run build will generate the new typedef file based on the above src file.

@skratchdot
Copy link
Owner

Nevermind! I figured out how to type this.$picker in commit 9322550.

I went ahead and published v7.0.0: https://www.npmjs.com/package/react-bootstrap-daterangepicker

I'm going to close this ticket. Please re-open if you encounter issues w/ v7. Thanks for helping out!

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

No branches or pull requests

3 participants