Unsafe accessing of nullable properties

[paulkaplan]

I found several places where we are accessing nullable properties in an unsafe way. A few of these were reported automatically through Sentry with type errors like Cannot read property 'rotationCenter' of null.

Drawable.js (mostly accessing properties of the skin without checking it exists)

• [ ] https://github.com/LLK/scratch-render/blob/8495c6c4e2bbb18a6637d3786db89e8b8070ab62/src/Drawable.js#L311-L318
• [ ] https://github.com/LLK/scratch-render/blob/8495c6c4e2bbb18a6637d3786db89e8b8070ab62/src/Drawable.js#L209
• [ ] https://github.com/LLK/scratch-render/blob/8495c6c4e2bbb18a6637d3786db89e8b8070ab62/src/Drawable.js#L217

@rschamp @cwillisf We tend to do this a lot in other repos (probably more than here, i'm not picking on render at all). It is a language trap that javascript lets you fall in easily and we aren't linting it (i'm not sure that is even possible?). Are there any design patterns we should think about that would help us from doing this type of thing?

[rschamp]

AFAIK the best safety against this kind of thing is a type-helper framework like Flow or Typescript. I think our best bet until we have something like that is tests that try to break things, and not assuming happy paths.

[cwillisf]

In my experience the only way that a Skin or Drawable becomes null is a bug in the VM. With that in mind, it might make sense to put checks for unexpected null or undefined values as far as possible toward the "surface" of the renderer API. We could even go as far as phrasing the messages like "Tried to (whatever) without a valid (whatever). Was a (sprite/costume) partially deleted?"

I do agree that a typing system would help, and there's a part of me that really likes the idea, but I also worry that it might scare off potential contributors.

Also, proper JSdoc can be a big help. I can already get some warnings in my IDE when the parameter types are set up accurately - things like ?Drawable cannot be converted to !Drawable for example.