fix: validate return type of external option callback#9545
Conversation
external option callback
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I’ve currently validated only the |
|
Wouldn't this have additional overhead because now we have a duplicate check? |
To be honest, this should be a very cheap check, so I don’t think we need to worry about it too much. |
|
For this specific case, I agree that it's cheap. But for example, the return value of |
My understanding is that we currently only check the top-level type, i.e., simple categories like |
Oh, really? I thought napi-rs checks the nested properties as well. If napi-rs's generated code only checks the top-level type, the only concern with this approach is how to ensure the code is synced with the actual types. |
Sorry, what I meant is that our |
|
But that means that the error message would be the same if the user returns a different nested type. If this is only for the top-level type, I'm not sure if it helps a lot. |
I still feel there's some mismatch here, since the Rust-side types don't always align with the JavaScript-side types, and there may be internal conversions happening. So do you think we should keep investing in this? |
|
If you don't have any ideas, let's hold off for now. |
Made it possible to add a message to the type error by `.context` and included the option name in it. close #9545
Summary
assertCallbackReturnutility to validate return values of user-provided option callbacks at the JS layer, before they cross to Rustexternaloption callback, producing a clear error likeInvalid return value for option "external": expected boolean, got string.instead of the cryptic NAPI errorValue is non of these types 'bool', 'UnknownReturnValue'string | string[]) for future use with other option callbacksCloses #9005