use tailored types instead of extending DOM #2174
Conversation
| @@ -80,7 +79,7 @@ export class Canvas { | |||
|
|
|||
| constructor(width: number, height: number, type?: 'image'|'pdf'|'svg') | |||
|
|
|||
| getContext(contextId: '2d', contextAttributes?: NodeCanvasRenderingContext2DSettings): NodeCanvasRenderingContext2D | |||
| getContext(contextId: '2d', contextAttributes?: NodeCanvasRenderingContext2DSettings): CanvasRenderingContext2D | |||
There was a problem hiding this comment.
This probably means a semver-major release, right?
There was a problem hiding this comment.
Hmm, might be worth keeping the old name then? 🤔
There was a problem hiding this comment.
We could also export aliases to the old names. I thought the "NodeCanvas" prefix was to avoid naming conflicts from lib.dom.ts.
Even then, it still could break someone's build if they were making function calls with arguments we ignore. Not sure if that's as big of a deal though?
There was a problem hiding this comment.
I'm not at all opposed to a semver-major release. This would be an easy one for users to handle. Just want to be on the same page.
Other question is usability - users could alias the type themselves (import type {CanvasRenderingContext2D as NodeCanvasRenderingContext2D}), but is it more convenient for the provided name to be NodeCanvasRenderingContext2D?
There was a problem hiding this comment.
I prefer the shorter name. We call it Canvas everywhere else, not NodeCanvas. Our main users are using node, so it shouldn't cause a naming conflict should it? If they do have DOM types, the imported type would take priority over the ambient types, which is what you'd expect. I'm not 100% certain it's the right call, but pretty sure it would be fine...
There was a problem hiding this comment.
Plus we already kind of did this. You can import {Image} which would conflict with the browser-defined Image.
There was a problem hiding this comment.
Fair point, same for the the gradient and pattern classes.
@LinusU good with you? Shorter name and semver-major?
There was a problem hiding this comment.
Hmm, it seems like the old code exported without the Node prefix, so I'm not sure this is a breaking change?
Line 210 in 672104c
There was a problem hiding this comment.
That's true. I think it would still let you import {NodeCanvasRenderingContext2D} from 'canvas' because it's a .d.ts. Could still export both. IMO the chances of this breaking someone's build are small as-is and we can only make it smaller but not get to 0.
There was a problem hiding this comment.
Good point! Makes more sense as semver-minor then.
chearon
force-pushed
the
ch/tailored-types
branch
from
1f6b018 to
3283240
Compare
The properties are ordered the same as their implementations, and I looked at each to make sure the signatures match. There are a lot of arguments in the DOM types that weren't supported by node-canvas, so this feels like the right way.
I tried to be careful, but this was a lot. I checked that every example file passes
// @ts-check. Even if there are no mistakes, since these new types are smaller, someone could have been passing arguments that node-canvas wouldn't recognize and it would break their typescript build.Fixes #1656