Those assignments just copy primitives, they don't provide references into the object.
You can (and should) minimize the problem down to:
const o = {foo: 42};
let {foo} = o;
foo = 600;
console.log(o); // what happened? I expected {foo: 600}...
Additionally, document.getElementById("GameView").getContext("2d");
returns a canvas context, not a canvas. It should be named as such, context
. It so happens you can grab the canvas from the context but it's sort of awkward and invasive: RenderClass.Canvas.canvas.width
(yikes...).
What's the point of this class exactly? If it's responsible for rendering, it shouldn't allow getters on canvas height and width -- those are properties of the canvas member, not the renderer. If you want to wrap the canvas class, write a class Canvas
that has these getters and setters, then offer a getter into the underlying canvas on the Renderer
class.
The syntax for setting width would be renderer.canvas.width = 42
rather than renderer.setWdith = 42
.
The other option would be to completely hide the canvas as an implementation detail of a renderer. In that design, the canvas would not be exposed at all, so the height/width getters/setters would make sense. But then there'd not be a getter for the canvas exposed by the Renderer
interface -- all interactions between the client and the underlying Renderer#canvas
would be mediated by the Renderer
interface.
Passing in an id to the constructor for RenderClass
is odd. There's no reason to tie down the class to only work with a subset of HTML elements that happen to have ids. A renderer doesn't need to know about the document
. Better is to pass in an arbitrary canvas element rather than baking getElementById
into the constructor. Think of it like the ReactDOM.render
call (if you're familiar with React), which lets the client specify the element. It'd be frustrating if React forced you to use an id and did the document traversal call under the hood -- we should be able to inject mocks into this.
As a final note, variables, including instance variables, should be named lowercase -- uppercase is for classes. CSS ids should be kebab case, #game-view
, not PascalCase. It's poor style to name a class FooClass
-- just Foo
. The capital is enough to distinguish it as a class.
Playing along with the class design (right now, I don't think you need classes at all, but I assume you have bigger abstractions to build), here's a rewrite suggestion:
class Canvas {
constructor(canvas) {
this._canvas = canvas;
this._context = canvas.getContext("2d");
}
get width() {
return this._canvas.width;
}
set width(width) {
this._canvas.width = width;
}
get height() {
return this._canvas.height;
}
set height(height) {
this._canvas.height = height;
}
// ...
}
class Renderer {
constructor(cavas) {
this._canvas = canvas;
}
get canvas() {
return this._canvas;
}
set canvas(canvas) {
this._canvas = canvas;
}
// ...
}
const canvas = new Canvas(document.querySelector("#game-view"));
const renderer = new Renderer(canvas);
const handleResize = () => {
renderer.canvas.width = window.innerWidth;
renderer.canvas.height = window.innerHeight;
console.log(
renderer.canvas.width,
renderer.canvas.height
);
};
window.addEventListener("resize", handleResize);
handleResize();
html, body, #game-view {
width: 100%;
height: 100%;
}
body {
margin: 0;
}
<canvas id="game-view"></canvas>
Don't forget to debounce your resize calls.