-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
TypeScript Rewrite #42
Conversation
Codecov Report
@@ Coverage Diff @@
## main #42 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 59 75 +16
Branches 6 9 +3
=========================================
+ Hits 59 75 +16
Continue to review full report at Codecov.
|
|
||
_excludeOurProps = () => { | ||
_excludeOurProps = (): SignaturePad.SignaturePadOptions => { |
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.
Instead of prefixing private properties or methods, public
and private
TS modifiers can be used.
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.
Yea I was thinking of doing that now that it's in TS, but I don't yet totally know all the ramifications or transpilation differences of private
or the new #
modifier
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.
Will be leaving this as is for now so that's it's not breaking any use of these that may have occurred. In particular, I've used and mentioned using _resizeCanvas
in the past.
Moving forward, should use JS-style #
for private fields
src/index.tsx
Outdated
// this is some hack-ish init and casting to avoid `| null` everywhere :/ | ||
/* eslint-disable @typescript-eslint/consistent-type-assertions */ | ||
_sigPad: SignaturePad = {} as SignaturePad | ||
_canvas: HTMLCanvasElement = {} as HTMLCanvasElement | ||
/* eslint-enable @typescript-eslint/consistent-type-assertions */ |
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 should best be avoided as it can lead to hard to track bugs in the future.
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.
It would require a ton of refactoring to have | null
work as there's accesses everywhere.
I'm open to hearing other options, but I don't think that refactoring is 1) worth the time or 2) worth the added complexity/worse readability
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.
After giving it some thought: IMO this is a breaking change. This changes how getSignaturePad
and getCanvas
public API works, as now it returns empty object instead of null in some rare cases.
A workaround like the following could be used to avoid redundant code:
private getSignaturePadInstance: () => {
if (this._sigPad == null) {
throw new Error('some meaningful message');
}
return this._sigPad;
}
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.
Yea I recognize it could be considered "breaking" in the strict sense, but so can lots of things -- ComVer exists for that reason.
Realistically that never happens and is definitely not behavior I'd encourage relying on or is really guaranteed by the library -- the docs do not mention that it might be null
and notably @types/react-signature-canvas
does not handle the null
case either.
I did consider adding a helper function, but that seemed far from ideal to me: to be explicit, adding | null
requires changes in 19+ places in the code, or ~15%+ of the source. Some of those changes would be non-trivial and confusing, and they'd hamper readability IMO.
I didn't quite consider throwing an error though, and that probably makes more sense than just doing nothing, especially as these cases are likely to error anyway.
But, throwing an error now can be considered a breaking change as well, particularly so in the sense you're describing.
I'm also not really sure what it should say, that's a React edge case and would prefer to throw a React error but idk. Possibly "rSC is mounting/unmounting right now, React refs are null during this phase".
Also the getSignaturePad
method already exists, there's no need for a separate method.
I don't know if there's a better solution; like I said, this seems far from ideal to me. Adding non-null assertion operators everywhere would be the least "change" but also not ideal.
The changes to width
/height
checks in _resizeCanvas
actually do have potential to break things which is why I'm almost certainly going to revert them.
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.
Ok, throwing an error is definitely a lot cleaner. In several cases can also just assign an alias const canvas = this.getCanvas()
to avoid repetition of the helper (but not in all places). Can push a change to this now, though still need to think about the ramifications of throwing an error.
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.
Throwing an error now. Seems ok. Only thing I wonder is if getCanvas()
and getSignaturePad()
shouldn't throw externally, and should just return null
like a ref would?
I think a custom error is a lot easier to understand than, like, accidentally calling a method on null
, but at the same time, changing the types to also return null
would be more explicit and prevent that case too 🤔
04ee418
to
b656bac
Compare
Working on getting this merged again. Shifted from TSDX (which I was solo maintainer of for over a year until my recent hiatus) to my Now comes the likely very painful rebase/merge process as |
I managed to rebase successfully after a heck of a lot of conflicts (practically all of the ~30 commits merged), esp. in the
Some other tiny changes I might want to make to this branch:
|
- adds a Dev CJS build, an ESM build, and some inferred typings - and has a similar Prod minified CJS build, which was the only build we had previously - no need for webpack production config anymore as TSDX handles that - add a tsconfig instead, in particular, ensure allowJs and esModuleInterop are true - switch .babelrc to .babelrc.js as TSDX is using Babel 7 everywhere - use TSDX's built-in babel config plus preset-react (deps): add TSDX and @babel/preset-react to support it - will begin removing deps in the next commit as other parts of the dev env are replaced by TSDX
- replace standard with eslint and all eslint-config-standard etc - hopefully tsdx lint should be more compatible with editors, tools, TS, etc than standard, because it gave lots of issues - lots of things, esp. integrations, are pretty unmaintained in standard's ecosystem :( - add react-app and prettier from tsdx lint --write-file so editors etc will be able to have the same config as TSDX internally - have to turn off prettier as double quotes and semicolons conflict with standard - fix lint errors in the example code (deps): add eslint-config-standard and all its peerDeps (deps): add eslint-plugin-react as tsdx lint will error without it
- TSDX changes some of Jest's defaults and so causes some issues for JS/JSX support - by default Jest supports JS and JSX in testMatch and babel-jest is automatically run on JS/JSX files if a babelrc is found - also had to configure class-properties for Babel as while it's automatically included with tsdx build, no Babel transforms are included with tsdx test (yet) (deps): remove jest as it's a dep of TSDX (deps): remove babel-jest@23 since we can use @24 now - which is a dep of jest@24 - remove require.resolve from it as it causes errors due to not being a direct dependency
- finally nix webpack@1 and babel@6 from the codebase - remove babel@6 presets, add @babel/core@7 - remove webpack loaders, parcel has built-in support for most - needed to add postcss-modules and a postcss.config.js to support the example's use of CSS modules - also fix a bunch of security warnings in the process - restructure example file hierarchy to be closer to CRA's for better standardization
- make heavy use of the multiple iterations on types in others' PRs, my own iterations on that, and then @types/react-signature-canvas - credit where credit is due! - NOTE: the naming of the props type and the class type do not include the word "React" unlike @types/react-signature-canvas - it is convention in React components to leave it out and you're already importing from a package with "react" in its name - refactor init logic a bit to get around `| null` everywhere - kinda hacky but better than ifs or ! (non-nullable) everywhere - use ! for canvas.getContext('2d') calls for same reason - c.f. microsoft/TypeScript#19229 - refactor naming everywhere to src/index.tsx - configure tsconfig -- no need for allowJs anymore, but jsx needs to be set as TS is actually reading it now - configure jest -- ts-jest needed to read TSX files (deps): add DT typings for prop-types, react, and signature_pad (deps): add a declaration file for trim-canvas as it doesn't have types built-in yet (TODO for me!) - use typings/ to hold external declarations
- not much to change except for some code to cast things to correct types - remove file extensions from TS imports as TS errors on them - configure .ts for ts-jest, and remove testMatch as we only have .spec.tsx? extensions now - leave out test/config/ set-up files as I'd like to split those out into separate packages anyway and their types aren't really necessary for test correctness (deps): add @types/enzyme for more explicit typings of tests
- for this commit, just focus on configuration and formatting errors, stricter typings will be in next commits - add tsconfig.eslint.json so that test/ files are linted too
- reuse React.Component types where possible, just like with the SignaturePad types
- use TS 3.0 defaultProps support so that clearOnResize doesn't have to be optional - ignore consistent-type-assertions and no-non-null-assertion in places where those hacks were intentionally used for simplicity's sake - fix some strict-boolean-expressions by checking for types explicitly or using nullish coalescing - nullish coalescing on canvasProps isn't breaking because it already had PropTypes.object defined - nullish coalescing on DPR isn't breaking because it should be a number (or undefined), and shouldn't be 0 - and even if it were 0, it would've been set to 1 before anyway bc of the Math.max - ignore several strict-boolean-expressions in _resizeCanvas because checking for undefined specifically is a breaking change - 0, '', null, etc falsey width/height was still resized previously, so if we check for undefined instead, it will stop resizing falseys - 0, '', null, etc don't make a lot of sense to set a width/height, if these were used it may have been accidental or for some temporary hack - accidental would be a bug, so will consider a fix, but fixing that bug would still be breaking :/ :/
- previously falsey values like 0, '', null, etc would have been resized, even though something like 0 _did_ set a fixed width/height - this could be considered breaking, so might leave out or revert this commit - and setting it to falsey may have intended it to be resized, bc setting it to 0 etc doesn't really make sense... but idk people can set whatever
- modify setRef to actually set internal refs to null if unmounted - modify getCanvas() and getSignaturePad() to throw errors if the ref is currently null - use both methods internally instead of directly accessing their respective variables - make some alias variables for brevity where possible - add a test for the error case as well
- we made it! woot! 🎉🎉
- because './src' is actually being used as the rootDir (deps): I deprecated this buggy option in TSDX but my PR has yet to be released - it outputs a warning in newer versions but no harm and only benefit in upgrading earlier
- I've been away from OSS for a while due to abuse and toxicity and no one's maintained TSDX in my time away, so lots of things are dated now - and TSDX was a large source of the above as well - and this library doesn't need everything TSDX provides -- most don't - use my boilerplate from https://github.com/agilgur5/ts-library-base - babel.config.js is basically a duplicate plus preset-react - tsconfig.json is dupe plus more excludes - tsconfig.build.json is dupe plus typings dir (and tsx ext for index) - jest.config.ts is dupe plus jsdom, enzyme, window-resizeto - rollup.config.ts is dupe minus CJS build and with some custom naming - similar for package.json#main,module,source,exports - fix: previously #main was changed to CJS, which could be considered breaking since it's currently UMD -- back to UMD now - package.json#scripts is a dupe plus lint and start - deps: add rollup, rollup-plugin-typescript2 for the config - deps: add @rollup/plugin-node-resolve, @rollup/plugin-commonjs, @rollup/plugin-babel, and rollup-plugin-terser for the build - and package-json-type and @babel/preset-typescript for typings - and @babel/runtime and @babel/plugin-transform-runtime for reusing Babel's runtime helpers instead of duplicating them - deps: add jest, jest-config, @jest/globals, @jest/types, ts-node for testing - RIP my jest-without-globals library - deps: add concurrently for parallel script execution - deps: add @tsconfig/strictest to extend tsconfig from - deps: add TS ofc - ci: add type-checking to before script check - ci: upgrade to Node 12 as oldest LTS - typings: improve typings with newer TS and stricter tsconfig - required `override` and required `import type` - fix(typings): move @types/react and @types/prop-types to deps - these are imported by the `.d.ts` declaration and even the DT lib: https://www.npmjs.com/package/@types/react-signature-canvas - deps: upgrade to lockfile v2 that is auto-used with NPM v8 - deps: upgrade @babel/core and @babel/preset-react to latest minor - and add @babel/preset-env as a direct devDep too - should upgrade compat-table etc and might as well do so to match minors of other babel deps and while changing so many deps anyway - deps: switch from eslint-config-standard-with-typescript to ts-standard - I didn't know this was a thing! standard-with-typescript was a relatively recent development in and of itself - should have used this from the beginning! - replaced all eslint deps with this one dep now - ignore the example dir for now as it errors (parsing?) and a line in the babel config as well (possibly due to old ESLint)
- deps: remove postcss-modules as Parcel no longer needs it - this also fixes 20+ vulns in the devDeps - use a namespace import for the CSS modules as requested in the docs: https://parceljs.org/languages/css/#css-modules - Per Migration docs (https://parceljs.org/getting-started/migration/): - use `type='module'` in `<script>` tag - use `.parcel-cache` in gitignore instead of `.cache` - misc: Babel is no longer necessary, so might move Parcel into the example dir instead, as it doesn't have to share deps now? - getting warnings from Parcel now that the Babel config is a deopt, but that's being used by Rollup (also still very fast) - misc: add `--open` to Parcel CLI script so it opens in browser window automatically
- fancy source maps for declarations - apparently I left these out of `ts-library-base` so they didn't get copied here either - but most of my tsconfig there was copied too, so I suspect I left it out either because of @wessberg/rollup-plugin-ts's bugs with it or because TSDX previously had bugs with it - c.f. ezolenko/rollup-plugin-typescript2#221 and the worse downstream version I had written first: jaredpalmer/tsdx#488 - Unfortunately I did encounter a small error with declaration maps when using rpts2 as a configPlugin, due to an unexpected edge case in code I wrote myself in the above PR - wrote up an issue for it and will probably PR it myself too: ezolenko/rollup-plugin-typescript2#310 - as a workaround, I wrote a small `tsconfig.rollup.json` to be used with rpts2 as a configPlugin that disables `declarationMap` - and also adds some optimizations over my base tsconfig - took me a bit to figure out how to use options with configPlugins, as that was one of the reasons I didn't use @rollup/plugin-babel as the configPlugin - I still can't get it to read my `babel.config.js` even with options as it has no option for this (it auto-detects it) :/
Ok everything is complete now, CI passes, and I've also made some more fixes here and there, including fixing some docs and adding Only thing worthy of note is that unlike |
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.
Looked through all the files a few more times and LGTM! 🎉
I've left two discussions unresolved here as there could be some follow-ups to them but I think the conclusions are satisfactory at least for now. This is going to be released as an alpha
first, so can reconsider some things before full release if there's negative user feedback as well.
Thanks again to ksocha for @types/react-signature-canvas
and a review here.
Fixes #21 , fixes #5 , replaces #25 , as well as the unofficial
@types/react-signature-canvas
/ DefinitelyTyped/DefinitelyTyped#41396This took a lot of hours, biggest change to this repo in a while. It probably took a decent amount longer to rip out & replace the entire development environment than it did to do the rewrite. Though the stricter type linting took a bit of time too.
Also contributed lots to TSDX to fix bugs and handle things, ended up contributing way more too and became a maintainer too 😅
One difference between what I said in #25 (comment) and what happened here was that I ended up using
parcel
for the example instead of CRA, because CRA kept giving errors about dependencies with different version numbers 😕 Parcel is cool and 0-config too though, as well as used by TSDX for some of its templates.Big thanks to @ksocha, who made
@types/react-signature-canvas
, for improving the types even further than the iterations I made on #25 and the original #5 . I re-used many of those here and improved them a bit further as well 🙂Also thanks to @ksocha for giving me some extra time to polish up & get this release out a bit behind schedule and while the demand was building up 🙌
Marked as WIP because there's some things from the
internal-standardization
branch that need to be PR'd first and I need to write up a release summary plus a few other things. That'll take some time especially because I very busy the next few days/week