-
Notifications
You must be signed in to change notification settings - Fork 27
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
implement and use DBOSJSON instead of JSON #490
implement and use DBOSJSON instead of JSON #490
Conversation
c4f423e
to
eb5f871
Compare
src/utils.ts
Outdated
return value; | ||
} | ||
|
||
const JSON_DATE_VALUE_PREFIX = 'date:' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the prefix could be more unique so we don't mistakenly deserialize other strings. For example, DBOSJsonDate
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
I was waiting for this.
Just name it!
Maybe add some of $%^@ as well.
What about using a library? |
Looks like this package doesn't support
We might find some better suited package later. |
Updated to use DBOSJsonDate as prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing with the team, we're reconsidering this PR -- we think a safer way is to parse into an object (similar to the way we implemented Buffer
replacer and reviver). Because the string with a prefix is still unsafe and will do a string matching for every single string.
After discussing with the team, we're reconsidering this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @demetris-manikas Thanks again for your contribution! We'd like to have two changes on this PR:
- Instead of prefixing a string, we think a safer way is to use an object, similar to the way we implemented
Buffer
support. For example:
interface SerializedDate {
dbos_type: 'dbos_Date';
dbos_data: string;
}
- Could you also replace
JSON
in all other places undersrc/
? There are still someJSON
in dbos-executor.ts, config.ts, fdb_system_database.ts, server.ts, and logs.ts.
Sure. |
007f3c6
to
5f715c2
Compare
Let's use |
Applied all recommendations |
This PR address #450
I chose the
getTime()
overtoUTCString()
since it consumes less space.