skratchdot / react-bootstrap-daterangepicker

A date/time picker for react (using bootstrap). This is a react wrapper around the bootstrap-daterangepicker project.
http://projects.skratchdot.com/react-bootstrap-daterangepicker/
Other
474 stars 203 forks source link

TS definitions broken since the `initialSettings` change. #224

Closed floriancargoet closed 4 years ago

floriancargoet commented 4 years ago

The recent initialSettings change has broken the TypeScript definitions.

Who is maintaining these type files?

skratchdot commented 4 years ago

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 commented 4 years ago

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: https://github.com/skratchdot/react-bootstrap-daterangepicker/issues/224#issuecomment-664488815

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Is there a PR with the updated typings anywhere?

skratchdot commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

skratchdot commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 7c941859969e98bedc902ac24a8dea2f1d34b108 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 commented 4 years ago

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

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!