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

Refactor typescript definitions, expose DayPickerInput #487

Merged
merged 2 commits into from Sep 30, 2017

Conversation

4 participants
@adidahiya
Contributor

adidahiya commented Aug 26, 2017

This PR:

  • Refactors typescript definitions into multiple files in the types/ folder
  • Exposes DayPickerInput types in a new file called DayPickerInput.d.ts
  • Supersedes #485

@gpbl @lpcarignan

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 26, 2017

Codecov Report

Merging #487 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #487   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         531    531           
  Branches      109    109           
=====================================
  Hits          531    531

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efd1d6f...9950795. Read the comment docs.

codecov bot commented Aug 26, 2017

Codecov Report

Merging #487 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #487   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         531    531           
  Branches      109    109           
=====================================
  Hits          531    531

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efd1d6f...9950795. Read the comment docs.

@lpcarignan

This comment has been minimized.

Show comment
Hide comment
@lpcarignan

lpcarignan Aug 29, 2017

I'm sorry to report that this does not work in my test sample project DayPickerInputType.

When I put the files of this PR in node_modules/react-day-picker/types, and I run tsc. I get the following errors:

main.tsx(8,28): error TS2497: Module '"DayPickerInputTypescript/node_modules/react-day-picker/types/index"' resolves to a non-module entity and cannot be imported using this construct.
main.tsx(9,33): error TS7016: Could not find a declaration file for module 'react-day-picker/DayPickerInput'. 'DayPickerInputTypescript/node_modules/react-day-picker/DayPickerInput.js' implicitly has an 'any' type.

I like your idea of splitting the type definitions into separate files.

I really appreciate your efforts @adidahiya at helping me with this. Would it be too much to ask you to test your pull request (or other solutions) in my sample Typescript project? After doing a npm install on this sample project, you can copy your files in node_modules/react-day-picker/types and run tsc.

While I understand export.module = DayPicker.default || DayPicker; should not change, I am of the opinion that naming it will make the Typescript compiler happy.

Finally, as my experience with Typescript/Javascript is limited, I am more than open to any other solution as long as it works in a Typescript project.

lpcarignan commented Aug 29, 2017

I'm sorry to report that this does not work in my test sample project DayPickerInputType.

When I put the files of this PR in node_modules/react-day-picker/types, and I run tsc. I get the following errors:

main.tsx(8,28): error TS2497: Module '"DayPickerInputTypescript/node_modules/react-day-picker/types/index"' resolves to a non-module entity and cannot be imported using this construct.
main.tsx(9,33): error TS7016: Could not find a declaration file for module 'react-day-picker/DayPickerInput'. 'DayPickerInputTypescript/node_modules/react-day-picker/DayPickerInput.js' implicitly has an 'any' type.

I like your idea of splitting the type definitions into separate files.

I really appreciate your efforts @adidahiya at helping me with this. Would it be too much to ask you to test your pull request (or other solutions) in my sample Typescript project? After doing a npm install on this sample project, you can copy your files in node_modules/react-day-picker/types and run tsc.

While I understand export.module = DayPicker.default || DayPicker; should not change, I am of the opinion that naming it will make the Typescript compiler happy.

Finally, as my experience with Typescript/Javascript is limited, I am more than open to any other solution as long as it works in a Typescript project.

@lpcarignan

This comment has been minimized.

Show comment
Hide comment
@lpcarignan

lpcarignan Sep 5, 2017

@adidahiya and @gpbl, I was wondering if there was still interest in regards to my last comment.

I can understand we all have other priorities with work so not a problem if my request is on the back burner. I just wanted to let you know that I'm still willing to collaborate and make react-day-picker easy to use in Typescript.

lpcarignan commented Sep 5, 2017

@adidahiya and @gpbl, I was wondering if there was still interest in regards to my last comment.

I can understand we all have other priorities with work so not a problem if my request is on the back burner. I just wanted to let you know that I'm still willing to collaborate and make react-day-picker easy to use in Typescript.

@gpbl gpbl added this to the v6.2.0 milestone Sep 27, 2017

@KyleGobel

This comment has been minimized.

Show comment
Hide comment
@KyleGobel

KyleGobel Sep 30, 2017

@lpcarignan
I was able import the date picker with typescript using the syntax:

import * as DayPicker from 'react-day-picker';
import * as DayPickerInput from 'react-day-picker/DayPickerInputer';

So everything seems to be working for me. I'm not quite sure how all the exports work, every project seems to do it a different way 🤷‍♂️

KyleGobel commented Sep 30, 2017

@lpcarignan
I was able import the date picker with typescript using the syntax:

import * as DayPicker from 'react-day-picker';
import * as DayPickerInput from 'react-day-picker/DayPickerInputer';

So everything seems to be working for me. I'm not quite sure how all the exports work, every project seems to do it a different way 🤷‍♂️

@lpcarignan

This comment has been minimized.

Show comment
Hide comment
@lpcarignan

lpcarignan Sep 30, 2017

@KyleGobel
Thx for letting me know it works in your project. There was a 6.1.1 release a few days ago.

Do you use the controls in something similar to this?

render(): JSX.Element {
        return (
            <div>
                <DayPicker
                    selectedDays={this.state.selectedDays}
                    onDayClick={this.handleDayClick.bind(this)}
                />
                <DayPickerInput value="MM/DD/YYYY" format="MM/DD/YYYY" />
                <p>
                    {this.state.selectedDays
                        ? this.state.selectedDays.toLocaleDateString()
                        : 'Please select a day 👻'}
                </p>
            </div>
        );
    }

lpcarignan commented Sep 30, 2017

@KyleGobel
Thx for letting me know it works in your project. There was a 6.1.1 release a few days ago.

Do you use the controls in something similar to this?

render(): JSX.Element {
        return (
            <div>
                <DayPicker
                    selectedDays={this.state.selectedDays}
                    onDayClick={this.handleDayClick.bind(this)}
                />
                <DayPickerInput value="MM/DD/YYYY" format="MM/DD/YYYY" />
                <p>
                    {this.state.selectedDays
                        ? this.state.selectedDays.toLocaleDateString()
                        : 'Please select a day 👻'}
                </p>
            </div>
        );
    }

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Sep 30, 2017

Owner

Hi everyone, so is this OK to merge? Is there a way to test it? I’m fine to merge it but I’d like to be sure it works as expected. I’d be happy to include this change in the upcoming release.
Sadly I’ve not the time to jump into TypeScript right now. Thanks for your patience!

Owner

gpbl commented Sep 30, 2017

Hi everyone, so is this OK to merge? Is there a way to test it? I’m fine to merge it but I’d like to be sure it works as expected. I’d be happy to include this change in the upcoming release.
Sadly I’ve not the time to jump into TypeScript right now. Thanks for your patience!

@lpcarignan

This comment has been minimized.

Show comment
Hide comment
@lpcarignan

lpcarignan Sep 30, 2017

@gpbl
I never had any feedback from my Aug 29 comment so I dropped the ball.

Based on @KyleGobel comment today, I'll find some time to test the latest version (6.1.1) of react-day-picker in my sample project DayPickerInputType.

I would be more than happy @gpbl if you could also test the modifications in this pull request in my sample project DayPickerInputType. I gave feedback from your team recommandations at the end of August in this pull request on this sample project but never got any feedback in return :-(

lpcarignan commented Sep 30, 2017

@gpbl
I never had any feedback from my Aug 29 comment so I dropped the ball.

Based on @KyleGobel comment today, I'll find some time to test the latest version (6.1.1) of react-day-picker in my sample project DayPickerInputType.

I would be more than happy @gpbl if you could also test the modifications in this pull request in my sample project DayPickerInputType. I gave feedback from your team recommandations at the end of August in this pull request on this sample project but never got any feedback in return :-(

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Sep 30, 2017

Owner

@lpcarignan thanks again for your effort, but I don't understand which action should I take from your feedback. I'll merge this as I need it for #501. If something will break I hope someone will fix it in a PR :)

Owner

gpbl commented Sep 30, 2017

@lpcarignan thanks again for your effort, but I don't understand which action should I take from your feedback. I'll merge this as I need it for #501. If something will break I hope someone will fix it in a PR :)

@gpbl gpbl merged commit 2ac28e0 into gpbl:master Sep 30, 2017

3 checks passed

ci/circleci: checkout-and-test Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing efd1d6f...9950795
Details
codecov/project 100% remains the same compared to efd1d6f
Details

@gpbl gpbl added the src:TypeScript label Sep 30, 2017

@adidahiya adidahiya deleted the adidahiya:ad/refactor-types branch Oct 2, 2017

gpbl added a commit that referenced this pull request Oct 5, 2017

Upgrade tests for React 16 (#501)
* Upgrade react deps

* Setup enzyme 3

* Remove console statements

* Refactor typescript definitions, expose DayPickerInput (#487)

* Refactor typescript definitions, expose DayPickerInput

* Add a comment

* Upgrade react deps

* Setup enzyme 3

* Remove console statements

* Remove @types/react

* Use react@16 for the examples

* Use pagedNavigation in months-reverse example

* Upgrade to latest enzyme

* Disable no-restricted-globals

* Use wrapper.update() in enzyme

See https://github.com/airbnb/enzyme/blob/master/docs/guides/migration-from-2-to-3.md#for-mount-updates-are-sometimes-required-when-they-werent-before

* Upgrade dependencies
@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Oct 5, 2017

Owner

Good news! Landed on v6.2 – thanks @adidahiya and @lpcarignan for the invaluable help 💟

Owner

gpbl commented Oct 5, 2017

Good news! Landed on v6.2 – thanks @adidahiya and @lpcarignan for the invaluable help 💟

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