Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: Add support for TypeScript scripts#477

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
kf6kjg wants to merge3 commits intoactions:main
base:main
Choose a base branch
Loading
fromkf6kjg:feature/typescript

Conversation

kf6kjg
Copy link

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 attributelanguage 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 usingtsx, 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.

nomeata, PKief, tkstang, Shinigami92, yamachu, massongit, vanessa, tsvetkovv, and okineadev reacted with hooray emoji
Ricky C added3 commitsJuly 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 ifactions#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.
@kf6kjgkf6kjg requested a review froma team as acode ownerJuly 6, 2024 02:36
@kf6kjg
Copy link
Author

kf6kjg commentedJul 8, 2024
edited
Loading

TODO: Should add info and examples to README.

tkstang reacted with thumbs up emoji

@kf6kjg
Copy link
Author

kf6kjg commentedJul 8, 2024
edited
Loading

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

  • Ascript block containingconst {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 aftertranspileModule interprets thescript 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.

@kf6kjgkf6kjg marked this pull request as draftJuly 9, 2024 15:39
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

1 participant
@kf6kjg

[8]ページ先頭

©2009-2025 Movatter.jp