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

[is-date-object] Add typing for is-date-object #33644

Merged
merged 12 commits into from Mar 18, 2019

Conversation

Projects
None yet
5 participants
@adamzerella
Copy link
Contributor

adamzerella commented Mar 6, 2019

@ljharb Adding some type defs for is-date-object ❤️

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If adding a new definition:

  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
  • If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
  • Create it with dts-gen --dt, not by basing it on an existing project.
  • tslint.json should be present, and tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.
@ljharb
Copy link
Contributor

ljharb left a comment

It’d be better to make multiple signatures - one that takes a Date and returns true, another that takes a union type of all non-date non-objects and returns false, and another that takes unknown and returns false.

Show resolved Hide resolved types/is-date-object/index.d.ts
@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 6, 2019

@adamzerella Thank you for submitting this PR!

Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes.

In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day!

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 6, 2019

@adamzerella The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@adamzerella adamzerella force-pushed the adamzerella:types/is-date-object branch from c12113c to 0e9ad09 Mar 6, 2019

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 6, 2019

@adamzerella One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 6, 2019

🔔 @ljharb - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Mar 6, 2019

unknown > any, but I'd still like to see multiple signatures (especially Date => true)

@adamzerella

This comment has been minimized.

Copy link
Contributor Author

adamzerella commented Mar 6, 2019

@ljharb Sorry man I pushed up one change and the bot fired, I'll add additional signatures over the next day or so, I think it's a good idea also 👍

@adamzerella adamzerella force-pushed the adamzerella:types/is-date-object branch from 0e9ad09 to 462c6f2 Mar 6, 2019

@adamzerella

This comment has been minimized.

Copy link
Contributor Author

adamzerella commented Mar 6, 2019

@ljharb Sorry man I pushed up one change and the bot fired, I'll add additional signatures over the next day or so, I think it's a good idea also 👍

@ljharb Updated 😄

type InvalidTypes = undefined | null | boolean | string | number | [] | {};

declare function isDateObject(value: Date): true;
declare function isDateObject(value: InvalidTypes | unknown): false;

This comment has been minimized.

@ljharb

ljharb Mar 6, 2019

Contributor

since the first one will match Dates, does that mean these will only match non-Dates?

I kind of think that unknown should always be Boolean, not false - since it’s unknown.

This comment has been minimized.

@adamzerella

adamzerella Mar 7, 2019

Author Contributor

Yeah anything other than a date should return a boolean - updated, I believe I've captured all the possible types other than Date in the InvalidTypes type

adamzerella added some commits Mar 6, 2019

@adamzerella adamzerella force-pushed the adamzerella:types/is-date-object branch from 462c6f2 to 0454d3a Mar 7, 2019

Show resolved Hide resolved types/is-date-object/index.d.ts Outdated

ljharb and others added some commits Mar 7, 2019

Update types/is-date-object/index.d.ts
Co-Authored-By: adamzerella <me@adamzerella.com>
@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 7, 2019

@adamzerella The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@gabritto
Copy link
Contributor

gabritto left a comment

Since this seems to be a popular package, it would be nice if we could accommodate users of older TS versions.

// Project: https://github.com/ljharb/is-date-object
// Definitions by: Adam Zerella <https://github.com/adamzerella>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 3.3

This comment has been minimized.

@gabritto

gabritto Mar 7, 2019

Contributor

Could you also add definitions compatible with older TypeScript versions? You can follow the instructions here: https://github.com/DefinitelyTyped/DefinitelyTyped/#i-want-to-use-features-from-typescript-31-or-above

This comment has been minimized.

@adamzerella

adamzerella Mar 12, 2019

Author Contributor

Good point, I've added simpler type defs for older versions of TS but I'm getting some weird compile errors:

expect  TypeScript@next compile error: 
Expected 1 arguments, but got 0

I've followed the docs and mimicked other TS projects with multiple definitions, is this a simple fix?

@adamzerella adamzerella force-pushed the adamzerella:types/is-date-object branch from 7aca09d to 093c68d Mar 11, 2019

Show resolved Hide resolved types/is-date-object/index.d.ts Outdated
Show resolved Hide resolved types/is-date-object/ts3.1/index.d.ts Outdated
Show resolved Hide resolved types/is-date-object/index.d.ts Outdated
@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 11, 2019

@adamzerella The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@adamzerella adamzerella force-pushed the adamzerella:types/is-date-object branch from 093c68d to fb7e15e Mar 12, 2019

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 12, 2019

@adamzerella The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 18, 2019

@adamzerella I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

@adamzerella

This comment has been minimized.

Copy link
Contributor Author

adamzerella commented Mar 18, 2019

@gabritto I've reverted all of the additional type compatibility changes in favour of a far simpler type definition. One that works between all versions of TS and gives developers a function definition.

I haven't been able to resolve type compatibility issues, I do believe that some types are better than nothing but I completely understand if @ljharb would prefer me to close this PR at this stage.

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 18, 2019

🔔 @gabritto - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Mar 18, 2019

Might as well land the simple one, and then open a followup - I’d like to see the more accurate type definitions that narrow better.

@typescript-bot typescript-bot moved this from Needs Author Attention to Review in Pull Request Status Board Mar 18, 2019

@sheetalkamat sheetalkamat merged commit 1cf1e67 into DefinitelyTyped:master Mar 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Pull Request Status Board automation moved this from Review to Done Mar 18, 2019

@typescript-bot

This comment has been minimized.

Copy link

typescript-bot commented Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.