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

Rebuild for TypeScript #514

Merged
merged 10 commits into from
Jul 26, 2021
Merged

Conversation

johnrom
Copy link
Contributor

@johnrom johnrom commented Jul 23, 2021

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

@johnrom johnrom mentioned this pull request Jul 23, 2021
@mmarkelov
Copy link
Collaborator

@johnrom thanks a lot for your work. I'll check this out

src/CountUp.tsx Outdated
Comment on lines 134 to 137
// instance.target is incorrectly marked private by typescript
if ((this.instance as any).target) {
this.instance?.reset();
}
Copy link
Collaborator

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);
  };

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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';
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 @@
{
Copy link
Collaborator

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

Copy link
Contributor Author

@johnrom johnrom Jul 25, 2021

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.

Copy link
Contributor Author

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> = {
Copy link
Collaborator

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

Copy link
Contributor Author

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.

@johnrom
Copy link
Contributor Author

johnrom commented Jul 26, 2021

Made some requested changes, an additional fix for Timeout, and pushed back on some other requested changes.

@johnrom
Copy link
Contributor Author

johnrom commented Jul 26, 2021

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",
Copy link
Collaborator

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

Copy link
Contributor Author

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.

@mmarkelov mmarkelov merged commit 565d2c0 into glennreyes:master Jul 26, 2021
@johnrom johnrom deleted the johnrom/typescript branch July 26, 2021 16:47
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.

Migrate to TypeScript
2 participants