Ban `instanceof` and avoid inheritance, specifically in `TypePattern`.

josdejong commented

2025-05-07 15:59:41 +00:00

Collaborator

In TypePatterns.js, there is a base class TypePattern and a couple of classes like MatchTypePattern that extend this base class. In general, I try to prevent using class inheritance when it's not really needed. Would it be possible in this case to remove the inheritance, and instead use TypeScript here and define TypePattern as an interface instead?

In `TypePatterns.js`, there is a base class `TypePattern` and a couple of classes like `MatchTypePattern` that extend this base class. In general, I try to prevent using class inheritance when it's not really needed. Would it be possible in this case to remove the inheritance, and instead use TypeScript here and define `TypePattern` as an interface instead?

glen commented

2025-05-10 00:31:52 +00:00

Owner

Well, I agree that at the moment inheritance is only used for the equality test. Is that not enough reason for using a class hierarchy? (I'll be frank that personally I would use a class hierarchy merely for the sake of having an instanceof test for all of the derived classes, if it were convenient, so it's hard for me to judge where the line is for when inheritance is "needed".)

If your judgment is that we should not use class inheritance here, then I will just make all the TypePatterns be entities that happen to have the same interface. In that case, is it OK if I just make each one its own non-derived class, or do you prefer I not use classes at all, but rather plain objects? Thanks for letting me know as soon as you have a chance so I can make the changes needed before going on to benchmarking.

Well, I agree that at the moment inheritance is only used for the equality test. Is that not enough reason for using a class hierarchy? (I'll be frank that personally I would use a class hierarchy merely for the sake of having an `instanceof` test for all of the derived classes, if it were convenient, so it's hard for me to judge where the line is for when inheritance is "needed".) If your judgment is that we should not use class inheritance here, then I will just make all the TypePatterns be entities that happen to have the same interface. In that case, is it OK if I just make each one its own non-derived class, or do you prefer I not use classes at all, but rather plain objects? Thanks for letting me know as soon as you have a chance so I can make the changes needed before going on to benchmarking.

josdejong commented

2025-05-14 13:31:20 +00:00

Author

Collaborator

I like the famous "prefer composition over inheritance"😄. I'm totally fine with using classes by themselves, but in my experience it helps to keep complexity managable by using inheritance as little as possible.

In general, I also try to separate data and logic as much as possible, and often the data is then just JSON which is easy to inspect, alter, serialize, etc. And the logic can often be plain JavaScript functions. That way there is not that much need for classes.

One remark regarding instanceof: in mathjs we do not rely on instanceof because that turned out to give issues in the past, see https://github.com/josdejong/mathjs/issues/345.

I like the famous "prefer composition over inheritance"😄. I'm totally fine with using classes by themselves, but in my experience it helps to keep complexity managable by using inheritance as little as possible. In general, I also try to separate data and logic as much as possible, and often the data is then just JSON which is easy to inspect, alter, serialize, etc. And the logic can often be plain JavaScript functions. That way there is not _that_ much need for classes. One remark regarding `instanceof`: in mathjs we do not rely on `instanceof` because that turned out to give issues in the past, see https://github.com/josdejong/mathjs/issues/345.

glen commented

2025-05-15 13:28:27 +00:00

Owner

Wow, OK, I didn't realize that instanceof`` was considered broken in JavaScript (I guess because if I create an object in one frame/window and it is somehow passed to another frame/window it is not an instanceof`` the versions of the classes in that other window? Is that the concerning issue?)

I will change TypePatterns to a collection of classes with the same interface and remove instanceof.

Wow, OK, I didn't realize that `instanceof`` was considered broken in JavaScript (I guess because if I create an object in one frame/window and it is somehow passed to another frame/window it is not an `instanceof`` the versions of the classes in that other window? Is that the concerning issue?) I will change TypePatterns to a collection of classes with the same interface and remove instanceof.

glen changed title from ~~Change TypePattern into an interface?~~ to Ban instanceof and avoid inheritance, specifically in TypePattern?

2025-05-15 13:29:06 +00:00

glen changed title from ~~Ban instanceof and avoid inheritance, specifically in TypePattern?~~ to Ban instanceof and avoid inheritance, specifically in TypePattern.

2025-05-15 13:29:14 +00:00

glen added the

priority

maintenance

labels 2025-05-15 13:29:22 +00:00

josdejong commented

2025-05-15 13:39:23 +00:00

Author

Collaborator

Yes indeed, instanceof doesn't reliably work between iframes and the main window.

Yes indeed, `instanceof` doesn't reliably work between iframes and the main window.

Rows
Columns

Ban instanceof and avoid inheritance, specifically in TypePattern. #33

Ban `instanceof` and avoid inheritance, specifically in `TypePattern`. #33