-
Notifications
You must be signed in to change notification settings - Fork 157
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
Added Typescript as a Compiler Target #165
base: master
Are you sure you want to change the base?
Conversation
The output of the new "TypeScript" target is typescript that uses a class based format instead of a prototype-based one. TypeScript allows for autocompletion, making it significantly easier to work with the output of complex formats. Certain shortcuts were also taken for the time being, namely using a type of 'any' instead of the 'type 1 | type 2' for switch types
shared/src/main/scala/io/kaitai/struct/languages/TypeScriptCompiler.scala
Outdated
Show resolved
Hide resolved
outHeader.puts("enumName?: string;") | ||
outHeader.puts("[key: string]: number | string | IDebug | IDebug[] | undefined;") | ||
outHeader.dec | ||
outHeader.puts("}") |
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.
It looks like this warrants to place it in runtime, not repeat it in every generated file?
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.
that might be a good idea, but the typescript runtime doesn't exist yet. Until it does, it's sadly necessary to include it in at least debug mode compilation
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.
Um, pardon me if I'm wrong, but from what I see so far, forking and implementing a TS runtime is 10x easier than what you've achieved already — and it will save from many existing kludges in JS runtime? Why not take that extra step — given that it's literally creation of 1 new repo with 1-2 files and that's it?
case _: EnumType => "enum" | ||
case ArrayType(innerType) => isUserOrEnumType(innerType) | ||
case _ => "other" | ||
} |
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.
Um, no. You don't really need any strings in there. If you really want this, you can perfectly return DataType
and match with these case _: UserType
in every place where you match with case "user"
, etc. OTOH, I'm not even sure why this is needed.
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.
Apologies, i'm not too familiar with scala (this is literally the only experience i've had with it so-far). I can definitely work on cleaning stuff up- I just needed to get this reviewed to understand what should be done and where
override def kstreamName: String = "KaitaiStream" | ||
|
||
// FIXME: probably KaitaiStruct will emerge some day in JavaScript runtime, but for now it is unused | ||
override def kstructName: 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.
Actually, it's a very good question. Probably TypeScript can make this interface a reality?
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 will likely happen once a runtime for typescript is created. I wasn't quite sure whether or not I should go ahead and name it so I just left it as-is
lang.readHeader(defEndian, seq.isEmpty) | ||
compileSeq(seq, defEndian) | ||
lang.readFooter() | ||
} |
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.
compileSeqProc
seems to be 100% equal of original implementation in ClassCompiler. Why would you need this override + copy/paste?
lang.attrParse(attr, attr.id, defEndian) | ||
wasUnaligned = nowUnaligned | ||
} | ||
} |
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.
Again, compileSeq
seems to be full equivalent of ClassCompiler implementation. Why do we need 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.
you're right on both accounts. I put them there initially because I thought that I would modify them somehow, however it turns out that's not the case.
shared/src/main/scala/io/kaitai/struct/translators/TypeScriptTranslator.scala
Outdated
Show resolved
Hide resolved
shared/src/main/scala/io/kaitai/struct/languages/TypeScriptCompiler.scala
Outdated
Show resolved
Hide resolved
shared/src/main/scala/io/kaitai/struct/languages/TypeScriptCompiler.scala
Outdated
Show resolved
Hide resolved
…ript works with all the tests that javascript can and more
The output of the new "TypeScript" target is typescript that uses a class based format instead of a prototype-based one.
TypeScript allows for autocompletion, making it significantly easier to work with the output of complex formats.
Certain shortcuts were also taken for the time being, namely using a type of
any
instead of thetype 1 | type 2
for switch types