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

Node canvas Path2D issue #36

Closed
gaitat opened this issue Mar 7, 2022 · 6 comments · Fixed by #43
Closed

Node canvas Path2D issue #36

gaitat opened this issue Mar 7, 2022 · 6 comments · Fixed by #43

Comments

@gaitat
Copy link

gaitat commented Mar 7, 2022

Can this package be used with a canvas that comes from a createCanvas() call of the canvas npm module?

When I try to use it as

import { Canvas, CanvasRenderingContext2D, createCanvas } from 'canvas';
import 'path2d-polyfill';

const canvasElement: Canvas = createCanvas(500, 500);
const context: CanvasRenderingContext2D = canvasElement.getContext('2d');
context.fill(new Path2D("M 80 80 A 45 45 0 0 0 125 125 L 125 80 Z"));

I get ReferenceError: Path2D is not defined

Thank you

@nilzona
Copy link
Owner

nilzona commented Apr 9, 2022

Hello @gaitat

Yes that should work. Can you give some more info on how and where you run this? Any other error you see?

@nilzona
Copy link
Owner

nilzona commented Apr 23, 2022

@gaitat have you tried to put import "path2d-polyfill" before all other imports (it should be the first in your bundle).

@LinusU
Copy link
Collaborator

LinusU commented Oct 11, 2022

It seems like this package checks for window, window.CanvasRenderingContext2D, and window.document.createElement.

You could potentially do something like this 👇 Probably easiest to do it as a separate file, that exports out what you need

const { Canvas, CanvasRenderingContext2D, createCanvas } = require('canvas')
globalThis.window = {}
globalThis.window.CanvasRenderingContext2D = CanvasRenderingContext2D
globalThis.window.document = {}
globalThis.window.document.createElement = () => createCanvas()
require('path2d-polyfill')
const Path2D = globalThis.window.Path2D

module.exports = { Canvas, CanvasRenderingContext2D, createCanvas, Path2D }

@nilzona we have a pull request Automattic/node-canvas#2013 (review) that seems to duplicate most of the code in here. What do you think about us moving just the implementation of Path2D to a separate repo, and then both this package and node-canvas could depend on it?

We have a organization node-gfx where we currently have implementations of Image, ImageData, and some other things. I think that Path2D would be a good fit to have there as well.

I'm thinking that that package would just export Path2D and isPointInPath. Then this package could use those exports to create the full polyfill for browsers, and node-canvas could use them to provide support for Path2D right out of the box.

(ping @zbjornson)

@nilzona
Copy link
Owner

nilzona commented Oct 11, 2022

hey @LinusU

so you want a way to get hold of Path2D in a node environment? sure I can look into that.

If that was the case, there's no need to move the code around .. right?

@LinusU
Copy link
Collaborator

LinusU commented Oct 11, 2022

The benefits of moving code around is as I see it:

  • We wouldn't need to include browser/polyfill specific code
  • We would also be maintainers of that package, so that we wouldn't be relying on a single person if there is a need to fix bug/cut releases

I should probably have explained it a bit more in the first post sorry about that. I also don't want to step on anyones toes, and I understand that it's a large ask to be added as co-maintainers. But I also didn't want to merge the PR we had which copies the code in (I wasn't aware the code where from here until just now), or fork the repo and remove the polyfill parts, without first having a discussion 😅

Potentially having a way to access Path2D and isPointInPath in a way that doesn't modify any globals or polyfills any functions in this package would be enough thought ☺️

@nilzona
Copy link
Owner

nilzona commented Oct 17, 2022

hey @LinusU I sent a mail to you about co-maintainer possibility. I sent it to linus@folkdatorn.se as it was your public e-mail in your github profile.

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 a pull request may close this issue.

3 participants