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

Fix incorrect Typescript types #62

Merged
merged 2 commits into from May 8, 2020

Conversation

henrinormak
Copy link
Contributor

@henrinormak henrinormak commented Feb 25, 2020

While trying to use this from TS, I noticed that I got the following error: a.default is not a function, which hinted at some import problem with this package.

After looking at the source code (specifically this line), it is evident that this package is what TS calls a "module library" - http://www.typescriptlang.org/docs/handbook/declaration-files/library-structures.html#identifying-a-module-library-from-code). Looking at other similar packages (namely request), you can see that their type definitions are not using export default.

As such, it seems that the types in this package should be changed:

declare function isEqual(a: any, b: any): boolean;
declare namespace isEqual {}
export = isEqual;

@chrisbolin chrisbolin added bug Something isn't working investigate Needs first level triage / investigation labels Apr 30, 2020
Copy link
Contributor

@kale-stew kale-stew left a comment

Choose a reason for hiding this comment

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

After some research, I'm going to say this LGTM. Express and other similarly simple, single fn-based libraries use the same type of declaration, and the TypeScript docs themselves appear to validate this approach.

Thank you for opening this PR, @henrinormak!

@kale-stew kale-stew merged commit c3c752b into FormidableLabs:master May 8, 2020
@chrisbolin
Copy link
Contributor

just as an example, here's express' type declarations in DefinitelyTyped

@chrisbolin
Copy link
Contributor

I think the original premise of this issue might be a little off.

a.default is not a function

That is correct: https://github.com/FormidableLabs/react-fast-compare/blob/master/index.js#L118
We don't do a default export. We instead assign module.exports to the function.

@chrisbolin
Copy link
Contributor

chrisbolin commented May 11, 2020

after looking at this, the key flaw that this PR fixes is

export default isEqual

There is no default export in the module, so the typing was off. Hence the export=isEqual

@chrisbolin
Copy link
Contributor

@henrinormak can you explain the purpose of

declare namespace isEqual {}

I'm pretty sure it's just dead code, especially after talking with @kitten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigate Needs first level triage / investigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants