-
Notifications
You must be signed in to change notification settings - Fork 131
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
Rebuild for TypeScript #514
Conversation
@johnrom thanks a lot for your work. I'll check this out |
src/CountUp.tsx
Outdated
// instance.target is incorrectly marked private by typescript | ||
if ((this.instance as any).target) { | ||
this.instance?.reset(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should simplify this to:
this.instance?.reset();
and we can skip creation of CountUp instance in createInstance
:
createInstance = () => {
if (typeof this.props.children === 'function') {
// Warn when user didn't use containerRef at all
warning(
this.containerRef.current &&
(this.containerRef.current instanceof HTMLElement ||
this.containerRef.current instanceof SVGTextElement ||
this.containerRef.current instanceof SVGTSpanElement),
`Couldn't find attached element to hook the CountUp instance into! Try to attach "containerRef" from the render prop to a an HTMLElement, eg. <span ref={containerRef} />.`,
);
return;
}
return createCountUpInstance(this.containerRef.current, this.props);
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is meant to be a types-only change and while I'm sure that check could be simpler, I'd recommend non-ts changes be added to my other PR moving to a hook-first approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the component CountUp must actually initialize the instance on mount for backwards compatibility - the tests all test for CountUp having 0 (props.start) in the component instead of an empty container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the this.instance check to the right spot (before checking this.instance.target). This createInstance function is actually for creating the CountUpJs instance, so I'm not sure where it would be created if not here.
src/CountUp.tsx
Outdated
@@ -1,9 +1,22 @@ | |||
import PropTypes from 'prop-types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that prop types should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah for sure. Feel free to make this change to my PR branch as I probably can't circle back until midweek.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed prop types.
@@ -1,13 +1,16 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep babel only for test files? I suppose that we can use ts-jest. But it can be done in separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC my implementation transpiles typescript using @babel/preset-typescript for both the build and tests. This is generally recommended instead of using the Typescript compiler when Babel is used for the test of transpilation instead of combining methods.
If I'm wrong about that I'll double check when I have some dev time.
Tsc is used to generate d.ts files themselves, as babel only transpiles to javascript.
I also recommend using the same transpilation method for jest as the project itself, so there aren't transpilation differences that cause test failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, this PR uses babel to transpile typescript for both build and test.
ref: string | React.RefObject<any>; | ||
} | ||
|
||
const defaults: Partial<useCountUpProps> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can move defaults
to common
file:
export const defaults = {
decimal: '.',
decimals: 0,
delay: null,
prefix: '',
separator: '',
start: 0,
startOnMount: true,
suffix: '',
useEasing: true,
}
and use it in both CountUp
and useCountUp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meant to be a ts-only PR, and I did make this change in my other PR where the component actually uses the hook.
…JS.Timeout in node, or number in the browser.
Made some requested changes, an additional fix for Timeout, and pushed back on some other requested changes. |
Also fixed the warning check in createInstance to cover all possible dom Elements. |
package.json
Outdated
"@babel/preset-typescript": "^7.14.5", | ||
"@rollup/plugin-node-resolve": "^13.0.2", | ||
"@rollup/plugin-typescript": "^8.2.3", | ||
"@types/warning": "^3.0.0", | ||
"countup.js": "^2.0.7", | ||
"prop-types": "^15.7.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i suppose that we can remove prop-types
from packages and rollback
config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
derp, yes I've now cleaned up package.json to put deps in the right place and remove @rollup/plugin-typescript
and prop-types
.
I noticed a few issues with the types when using this in a typescript project, then I saw the issue here: #109, so I thought I might do that and fix a few things. The most glaring issue was that ref was not a part of the typescript definitions I had for useCountUp.
closes #109