-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Compiler: Support for stdin TypeScript source code #5246
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
Conversation
internal/js/compiler/compiler.go
Outdated
| return nil, "", err | ||
| } | ||
|
|
||
| if isTsExtensionFile { |
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 am not certain we should report the stdin parsing as well ... if we are going to do it.
I guess we can only report it past the StripTypes if the worry is that this might not work and we will report stuff more?
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 reason why I left it as-is is because I think it keeps the current behavior, on which we were only reporting usage when the file extension is indeed .ts.
But yeah, I agree that we could move that to once we have stripped types with no error (err == nil), so that way we would report it in both cases. Is that what you mean? Do you also think that would make sense?
If so, I'm happy to add that change here, before merging 👍🏻
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.
yeah 👍
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.
LGTM, left a usage related, not blocking comment
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.
LGTM if CI is going to be green
What?
Instead of stripping types (github.com/evanw/esbuild) only when the file extension is
.ts, it also does it (or tries to) when the script is provided directly through stdin (instead of file) and it cannot be parsed as JavaScript/ECMAScript source.Why?
Because currently, if the user provides TS source directly through stdin, k6 throws an error -the one it gets when trying to parse that code as JavaScript/ECMAScript source- and never tries to parse it as TS.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #4773 and #5210