-
Notifications
You must be signed in to change notification settings - Fork 71
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
type inconsistency in payloads #94
Comments
I was just looking around today, checking if this had been mentioned already. My
and because there is only one PayloadType, the toIntIfInt function wrongly converts it into an I wonder if the format grammar for the Current workaround: force a string on those fields: // Parse the SDP message text into an object.
const sdpObj = SdpTransform.parse(sdpStr);
// Force "payloads" to be a string field.
for (const media of sdpObj.media) {
media.payloads = "" + media.payloads;
} |
I tried to reproduce these issues, but I couldn't. Hope you fixed it or if not appreciate it if you provide the SDP sample. |
This is trivial to reproduce. Here's a minimal sample that shows the issue, I hope it is useful for understanding the bug: File "use strict";
const SdpTransform = require("sdp-transform");
const util = require("node:util");
const sdp = `
m=audio 1 UDP 11 22
m=video 1 UDP 33
`;
const sdpObj = SdpTransform.parse(sdp);
console.log("sdpObj:", util.inspect(sdpObj));
console.log()
console.log("sdpObj.media[0].payloads:", util.inspect(sdpObj.media[0].payloads));
console.log("sdpObj.media[1].payloads:", util.inspect(sdpObj.media[1].payloads));
console.log()
console.log("typeof sdpObj.media[0].payloads:", typeof sdpObj.media[0].payloads);
console.log("typeof sdpObj.media[1].payloads:", typeof sdpObj.media[1].payloads); Run commands: mkdir /tmp/issue-94 && cd /tmp/issue-94
npm install sdp-transform
touch main.js # Copy contents from above.
node main.js Obtained output:
Expected output:
|
It's not unexpected. If a value can be converted into a number, it is converted into a number. If not, it's becomes a string. That's how all things work in the parser. Whether that's super nice or not... that's another story. |
yeah, true. it's not great, but it is what it is. |
I meant "Expected output" as in "expected if there were no type inconsistency as reported in this issue". It is for sure a bit strange that the field could be of one type or another, depending on the input given. And I agree it's not super nice, and consistency would be very much desirable. |
The issue seems to be the assumption that The ideal solution might be a map of specific type-per-field, enforced for each of them. But implementing all of that just for a single field... makes me feel like the kludge of manually forcing a string type just for that field might not be too bad, and would solve the issue. (But if more fields like that one start being discovered... yeah a map of types would be better). Can any other instances of this issue happen? |
Sure. |
Yes, as you mentioned |
When the payload is only one, a number is returned. when there are multiple payloads a string is returned.
I think we should have a consistency to be able to safely parse those fields
See the following example:
The text was updated successfully, but these errors were encountered: