-
Notifications
You must be signed in to change notification settings - Fork 6
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
datetime and dateOnly types should be mapped to TypeScript Dat #72
Conversation
@@ -67,11 +66,11 @@ class PackageProvider @Inject constructor( | |||
|
|||
private inner class ResourcePackageSwitch : ResourcesSwitch<String>() { | |||
override fun caseMethod(`object`: Method): String { | |||
return "$clientPackage.resource" | |||
return "$clientPackage" |
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.
This change was required because it broke our current code, don't know why we added this in the first place.
} | ||
|
||
override fun caseResource(`object`: Resource?): String { | ||
return "$clientPackage.resource" | ||
return "$clientPackage" |
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.
This change was required because it broke our current code, don't know why we added this in the first place.
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.
Looks good for me 👍 , i remember introducing this just for path cosmetics but if it causes errors we re better off without it 😄
just a small remark bellow ⬇️
@@ -10,8 +10,8 @@ object TypeScriptBaseTypes : LanguageBaseTypes( | |||
doubleType = nativeTypeScriptType("number"), | |||
stringType = nativeTypeScriptType("string"), | |||
booleanType = nativeTypeScriptType("boolean"), | |||
dateTimeType = nativeTypeScriptType("string"), |
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.
This one wouldn't work since the string date is interpreted as string so trying to call a date method will induce type error.
@acbeni I agree with you that it currently doesn't make a lot of sense to change the mapping, because But there are already some TypeScript JSON serializer/deserializers that can take advantage of the type info. The most popular is inspired by jackson and seems to be stable: https://github.com/shakilsiraj/json-object-mapper For now I would suggest to remove the mapping, but keep the other fix. May be with another branch? WDYT? |
removing the mapping seems good for now, what i would suggest is to create another issue, because it would be interesting to see whats the best approach is: use external library (which might be bad for because it make us also related to their vulnireabilities), or introduce our own utils for serialization de-serialization. |
Closes #71