search

code-dot-org / dance-party

Renderer for the Dance game type. Based on p5.js and p5.play.js.
https://code-dot-org.github.io/dance-party/
13 stars 13 forks source link

API validation warnings in Dance throw exceptions instead of warning #132

Open Bjvanminnen opened 6 years ago

Bjvanminnen commented 6 years ago

We recently ran into an issue where our api validation was failing

https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/lib/util/audioApi.js#L42

You would expect to see the appropriate error logged to the console. Instead, we were seeing an exception.

This is because we were ultimately calling outputWarning here https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/lib/util/javascriptMode.js#L25 and errorHandler is undefined.

I suspect we want to call injectErrorHandler in dance, similar to how we do in GameLab, i.e. https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/gamelab/GameLab.js#L204

ryansloan commented 6 years ago

@joshlory can you chime in with your thoughts on priority of this? I don't have enough context to understand the implications

joshlory commented 6 years ago

Agree we should fix this. I don't know if it meets the bar for Launch Day — hopefully the interpreted code defends against the user writing any code that could throw, but a bit concerning that we're getting exceptions from playSound in interpreted code.

@Bjvanminnen can you share the repro where the validation API was failing?

Bjvanminnen commented 6 years ago

I dont have an active repro. I'd hit this before https://github.com/code-dot-org/code-dot-org/pull/25516/files, I think because we'd made a change to dance that was still waiting on the corresponding change to the CDO repo.

ghost commented 6 years ago

i don't know what priority this should be, but marking as csedweek bug given the conversation above.