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

[geometry-1] Methods of immutable classes should return immutable instances. #586

Open
trusktr opened this issue Dec 28, 2024 · 0 comments

Comments

@trusktr
Copy link

trusktr commented Dec 28, 2024

For example, DOMMatrixReadOnly.multiply should return DOMMatrixReadOnly instead of DOMMatrix.

This makes the API awkward to use when choosing immutability:

const mat1 = new DOMMatrixReadOnly(...)
const mat2 = new DOMMatrixReadOnly(...)
const mat3 = new DOMMatrixReadOnly(mat1.multiply(mat2).toFloat64Array())

Not only is this awkward, but it wastefully creates 4 matrices and an unnecessary typed array, instead of only 3 matrices, because mat1.multiply(mat2) returns a DOMMatrix, so we need to call toFloat64Array to pass the array to DOMMatrixReadOnly to create the immutable isntance.

Furthermore, the way the APIs currently work prevents people from using the APIs for subclasses. 🥲

class MyMatrix extends DOMMatrix {
  specialNewMethod() {
    const newMat = this.rotateAxisAngle(...)
    // do something with newMat
    return newMat
  }
}

Now the user's new specialNewMethod will erroneously return a DOMMatrix instance instead of a MyMatrix instance, forcing the user to make the code more expensive again like this:

class MyMatrix extends DOMMatrix {
  specialNewMethod() {
    const newMat = this.rotateAxisAngle(...)
    // do something with newMat
    return new this.constructor(newMat.toFloat64Array())
  }
}

Instead, the ideal API would be like this:

const mat1 = new DOMMatrixReadOnly(...)
const mat2 = new DOMMatrixReadOnly(...)
const mat3 = mat1.multiply(mat2)

If the browser's implementation of multiply (and other immutable methods) were to use the equivalent of new this.constructor(), making subclasses would be simpler, and immutability would also be preserved as expected.

Related:

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

No branches or pull requests

1 participant