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.

Sign up for GitHub

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

Jump to bottom

Rebuild for TypeScript #514

Merged
merged 10 commits into from
Jul 26, 2021
Merged

Rebuild for TypeScript #514

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
770c2ec
Typescript-ify.
johnrom Jul 22, 2021
ab1247c
Add TS to build process.
johnrom Jul 23, 2021
b3a9f47
Fix build script.
johnrom Jul 23, 2021
a350e77
Remove proptypes.
johnrom Jul 26, 2021
840fe99
Timeout should use the return type of setTimeout, as it could be Node…
johnrom Jul 26, 2021
9e9a18f
Check for existing instance before checking for instance.target.
johnrom Jul 26, 2021
7515613
Fixed createInstance Element warning.
johnrom Jul 26, 2021
6b1f465
Fixed createInstance element warning (2)
johnrom Jul 26, 2021
5b9412a
Clean up package.json.
johnrom Jul 26, 2021
cdad744
Remove prop-types from rollup config.
johnrom Jul 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions babel.config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 •
edited
Loading

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.

"plugins": ["@babel/plugin-proposal-class-properties"],
"presets": [
["@babel/preset-env", { "modules": false }],
"@babel/preset-react"
"@babel/preset-react",
"@babel/preset-typescript"
],
"plugins": ["@babel/plugin-proposal-class-properties"],
"env": {
"test": {
"plugins": ["@babel/plugin-proposal-class-properties"],
"presets": [["@babel/preset-env", { "targets": { "node": "current" } }]]
"presets": [
["@babel/preset-env", { "targets": { "node": "current" } }]
]
}
}
}
85 changes: 0 additions & 85 deletions index.d.ts
View file
Open in desktop

This file was deleted.

12 changes: 8 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,25 @@
"homepage": "https://react-countup.now.sh",
"main": "build",
"files": [
"build",
"index.d.ts"
"build"
],
"typings": "index.d.ts",
"typings": "build/index.d.ts",
"scripts": {
"build": "rollup -c",
"build": "rollup -c && tsc --emitDeclarationOnly --noEmit false --project src/tsconfig.json --outDir build",
"prepublishOnly": "yarn build",
"test": "jest"
},
"peerDependencies": {
"react": ">= 16.3.0"
},
"dependencies": {
"@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.

"typescript": "^4.3.5",
"warning": "^4.0.3"
},
"devDependencies": {
Expand Down
7 changes: 6 additions & 1 deletion rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
import babel from 'rollup-plugin-babel';
import resolve from '@rollup/plugin-node-resolve';

const extensions = ['.js', '.jsx', '.ts', '.tsx'];

export default {
input: 'src/index.js',
input: 'src/index.ts',
output: {
file: 'build/index.js',
format: 'cjs',
},
plugins: [
resolve({ extensions }),
babel({
exclude: 'node_modules/**',
extensions,
}),
],
external: ['countup.js', 'prop-types', 'react', 'react-dom', 'warning'],
Expand Down
56 changes: 35 additions & 21 deletions src/CountUp.js → src/CountUp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

import React, { Component } from 'react';
import React, { Component, CSSProperties } from 'react';
import warning from 'warning';
import { createCountUpInstance } from './common';
import { CountUp as CountUpJs } from 'countup.js';
import { CallbackProps, CommonProps, RenderCounterProps } from './types';

export interface CountUpProps extends CommonProps, CallbackProps {
className?: string;
redraw?: boolean;
preserveValue?: boolean;
children?: (props: RenderCounterProps) => React.ReactNode;
style?: CSSProperties;
}

class CountUp extends Component<CountUpProps> {
private instance: CountUpJs | undefined;
private timeoutId: NodeJS.Timeout | undefined;

class CountUp extends Component {
static propTypes = {
decimal: PropTypes.string,
decimals: PropTypes.number,
Expand Down Expand Up @@ -58,7 +71,7 @@ class CountUp extends Component {
this.start();
}

checkProps = (updatedProps) => {
checkProps = (updatedProps: CountUpProps) => {
const {
start,
suffix,
Expand Down Expand Up @@ -86,20 +99,20 @@ class CountUp extends Component {
return hasPropsChanged || redraw;
};

shouldComponentUpdate(nextProps) {
shouldComponentUpdate(nextProps: CountUpProps) {
const { end } = this.props;
return this.checkProps(nextProps) || end !== nextProps.end;
}

componentDidUpdate(prevProps) {
componentDidUpdate(prevProps: CountUpProps) {
// If duration, suffix, prefix, separator or start has changed
// there's no way to update the values.
// So we need to re-create the CountUp instance in order to
// restart it.
const { end, preserveValue } = this.props;

if (this.checkProps(prevProps)) {
this.instance.reset();
this.instance?.reset();
this.instance = this.createInstance();
this.start();
}
Expand All @@ -108,18 +121,19 @@ class CountUp extends Component {
// end value.
if (end !== prevProps.end) {
if (!preserveValue) {
this.instance.reset();
this.instance?.reset();
}
this.instance.update(end);
this.instance?.update(end);
}
}

componentWillUnmount() {
if (this.timeoutId) {
clearTimeout(this.timeoutId);
}
if (this.instance.target) {
this.instance.reset();
// 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.

}

Expand All @@ -141,18 +155,18 @@ class CountUp extends Component {
const { reset, restart: start, update } = this;
const { onPauseResume } = this.props;

this.instance.pauseResume();
this.instance?.pauseResume();

onPauseResume({ reset, start, update });
onPauseResume?.({ reset, start, update });
};

reset = () => {
const { pauseResume, restart: start, update } = this;
const { onReset } = this.props;

this.instance.reset();
this.instance?.reset();

onReset({ pauseResume, start, update });
onReset?.({ pauseResume, start, update });
};

restart = () => {
Expand All @@ -164,28 +178,28 @@ class CountUp extends Component {
const { pauseResume, reset, restart: start, update } = this;
const { delay, onEnd, onStart } = this.props;
const run = () =>
this.instance.start(() => onEnd({ pauseResume, reset, start, update }));
this.instance?.start(() => onEnd?.({ pauseResume, reset, start, update }));

// Delay start if delay prop is properly set
if (delay > 0) {
if (delay && delay > 0) {
this.timeoutId = setTimeout(run, delay * 1000);
} else {
run();
}

onStart({ pauseResume, reset, update });
onStart?.({ pauseResume, reset, update });
};

update = (newEnd) => {
update = (newEnd: string | number) => {
const { pauseResume, reset, restart: start } = this;
const { onUpdate } = this.props;

this.instance.update(newEnd);
this.instance?.update(newEnd);

onUpdate({ pauseResume, reset, start });
onUpdate?.({ pauseResume, reset, start });
};

containerRef = React.createRef();
containerRef = React.createRef<any>();

render() {
const { children, className, style } = this.props;
Expand Down
5 changes: 4 additions & 1 deletion src/common.js → src/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { CountUp } from 'countup.js';
import { CountUpProps } from './CountUp';
import { useCountUpProps } from './useCountUp';

export const createCountUpInstance = (el, props) => {
export const createCountUpInstance = (el: string | HTMLElement | HTMLInputElement, props: useCountUpProps | CountUpProps) => {
const {
decimal,
decimals,
Expand All @@ -15,6 +17,7 @@ export const createCountUpInstance = (el, props) => {
suffix,
useEasing,
} = props;

return new CountUp(el, end, {
startVal: start,
duration,
Expand Down
0 src/index.js → src/index.ts
View file
Open in desktop
File renamed without changes.
24 changes: 24 additions & 0 deletions src/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"compilerOptions": {/* Basic Options */
"target": "es5", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', or 'ESNEXT'. */
"module": "commonjs", /* Specify module code generation: 'commonjs', 'amd', 'system', 'umd' or 'es2015'. */
"jsx": "react", /* Specify JSX code generation: 'preserve', 'react-native', or 'react'. */
"declaration": true, /* Generates corresponding '.d.ts' file. */
"noEmit": true, /* Do not emit outputs. */

/* Strict Type-Checking Options */
"strict": true, /* Enable all strict type-checking options. */
"noImplicitAny": true, /* Raise error on expressions and declarations with an implied 'any' type. */
"strictNullChecks": true, /* Enable strict null checks. */
"noImplicitThis": true, /* Raise error on 'this' expressions with an implied 'any' type. */
"alwaysStrict": true, /* Parse in strict mode and emit "use strict" for each source file. */

/* Additional Checks */
"noUnusedLocals": true, /* Report errors on unused locals. */
"noUnusedParameters": true, /* Report errors on unused parameters. */
"noImplicitReturns": true, /* Report error when not all code paths in function return a value. */
"noFallthroughCasesInSwitch": true, /* Report errors for fallthrough cases in switch statement. */

"esModuleInterop": true,
}
}
Loading