Skip to content
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

feat: Add support for TypeScript scripts #477

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kf6kjg
Copy link

@kf6kjg kf6kjg commented Jul 6, 2024

Both of the .cts and .mts flavors. Because this action is written in CommonJS both have to compile to CommonJS in order to execute.

To handle this a new optional attribute language was added that defaults to the current logic of loading the script as CommonJS. Users can thus opt-in to the TypeScript variants if they wish.

My commits each do a separate transform of the codebase. The first is merely a minor cleanup of the existing tests. The second does a more complex implementation of TypeScript support that I personally prefer, but that may have to wait until the next major revision and a conversion to ESM. The third commit alters the second such that it re-uses the existing load-as-function logic after the transpile.

I explored using tsx, but while there's some hacky ways to load scripts into it via data URLs, I liked the pure typescript transpile approach better as I wasn't having to do any hacks or rely on a new library.

Ricky C added 3 commits July 5, 2024 19:05
`fn.name` was introduced in Node 0.10.0, so this improves test suite maintainability without changing the minimum node version.

Also added a test that for certain proves that Promises can be awaited.
Both of the .cts and .mts flavors. Because this action is written in CommonJS both have to compile to CommonJS in order to execute.

As it is TypeScript there's already an expectation of some slowness, so I went with the approach of running the script via the node VM module. While a cleaner approach, it has the caveat that root level await in the script doesn't work. That should become available if actions#457 is completed.
We lose the more normal looking export in favor of the `return` statement already traditional to this action, and thus can handle await statements without an ESM conversion.

This is a separate commit from the former because I think the next major version of this action should switch to ESM, revert this commit, and use the more standard export notation in all supported languages.
@kf6kjg kf6kjg requested a review from a team as a code owner July 6, 2024 02:36
@kf6kjg
Copy link
Author

kf6kjg commented Jul 8, 2024

TODO: Should add info and examples to README.

@kf6kjg
Copy link
Author

kf6kjg commented Jul 8, 2024

Sadness, this isn't working as well as I'd hoped. I'd gone with transpileModule as it allowed me to skip the whole send-to-disk-and-load pipeline. BUT it turns out that transpileModule is single-file only. It has no ability to transform any imported files. Thus the following scenario fails with SyntaxError: Unexpected token ':':

  • A script block containing const {test} = await import('${{ github.workspace }}/importable.cts').
  • A file located at ${{ github.workspace }}/importable.cts with the following content: function a(): string { return "s" }; a()
    This is because Node is at the helm after transpileModule interprets the script block and is assuming that all loaded files are JS.

Bummer, I get to rewrite - what's the point of TS if you can't import more TS? That's MY primary use case.

@kf6kjg kf6kjg marked this pull request as draft July 9, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant