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

Export more types from index.d.ts #618

Merged
merged 1 commit into from
Feb 20, 2018
Merged

Export more types from index.d.ts #618

merged 1 commit into from
Feb 20, 2018

Conversation

adidahiya
Copy link
Contributor

Fixes #617

@codecov
Copy link

codecov bot commented Jan 31, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #618   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         621    621           
  Branches      133    133           
=====================================
  Hits          621    621

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 3156e3d...1668200. Read the comment docs.

@@ -2,3 +2,6 @@

import DayPicker from './DayPicker';
export default DayPicker;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you add export * from './DayPickerInput'; it'll also solve #586

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is semantically incorrect. DayPickerInput is not exported from the root of the package: https://github.com/gpbl/react-day-picker/blob/master/DayPicker.js

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, what would be the correct way then? Kinda new to typescript.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the JS export is from a submodule, you can import in typescript like this:

import { DayPickerInput } from "react-day-picker/DayPickerInput";

that's why types/DayPickerInput.d.ts exists.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing like that doesn't work, my compiler complains about no declaration file found.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, ok, can you file an issue for that? I might be able to look into it later

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I open up a new issue or is #586 good enough? Thank you!

@alexisthual
Copy link

Can someone merge this please? :)

@gpbl
Copy link
Owner

gpbl commented Feb 20, 2018

Hey thanks for the PR! Are we safe with this? I'm not a TS developer, people were fighting for this kind of issues the last time... should I go and merge?

@alexisthual
Copy link

I am not a TS expert but this PR seems quite fair to me; you can go ahead and merge I guess :)

@adidahiya
Copy link
Contributor Author

This is safe; I contributed to the original types on DefinitelyTyped and in this repo. #586 is a separate issue and can be addressed in another PR.

@gpbl
Copy link
Owner

gpbl commented Feb 20, 2018

Thanks @adidahiya as always!

@gpbl gpbl merged commit f923000 into gpbl:master Feb 20, 2018
@adidahiya adidahiya deleted the patch-1 branch February 20, 2018 17:31
@gpbl gpbl added this to the next patch milestone Feb 23, 2018
@gpbl gpbl added the v:minor label Feb 23, 2018
@gpbl gpbl removed this from the next patch milestone Feb 23, 2018
@gpbl gpbl added v:patch and removed v:minor labels Feb 25, 2018
@gpbl gpbl added this to the v7.1.0 milestone Mar 5, 2018
@gpbl
Copy link
Owner

gpbl commented Mar 5, 2018

Published as v7.1.0.

@theigor
Copy link

theigor commented Mar 5, 2018

Hi guys - just got latest and getting this error:

ERROR in [at-loader] ./node_modules/react-day-picker/types/props.d.ts:177:27
    TS7006: Parameter 'DayModifiers' implicitly has an 'any' type.

where line 177 is

  onDayChange?(day: Date, DayModifiers): void;

so this seems like a legitimate issue. Did you mean to do

  onDayChange?(day: Date, modifiers:DayModifiers): void;

@theigor
Copy link

theigor commented Mar 5, 2018

By the way, you can avoid things like this in the future by switching your tsconfig to

{
  "compilerOptions": {
    "noImplicitAny": true,
    ...
  },
  ...
}

kimamula pushed a commit to kimamula/react-day-picker that referenced this pull request Aug 17, 2022
Export more types from index.d.ts
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.

5 participants