convert the implementation of the action to TypeScript #75
Conversation
|
@DanTup - before I ask for a general review of the implementation and such - do you mind looking over the typescript code here, and the various typescript config files? It's been a while since I've written typescript, and it would be good to have somebody w/ domain expertise look the files over :) |
There was a problem hiding this comment.
All looks good to me. I'm not completely familiar with all the latest TS config options/files (my experience is mostly only setting up Dart-Code and keeping it running), but what's there seems reasonable (esp. if the TS action was used as a base).
Added a few questions/nits.
|
Looks pretty nice to me. The |
| if (fs.existsSync(versionFilePath)) { | ||
| return fs.readFileSync(versionFilePath, 'utf8').trim() | ||
| } else { | ||
| return null |
There was a problem hiding this comment.
Should we throw here? Or is there a legit case where there is no version file?
The action will end up storing an empty string in the output in this case:
https://github.com/actions/toolkit/blob/0db3029fcfcb1393a2b3ccd14caecd7a3460af4e/packages/core/src/utils.ts#L11
| import * as versions from './versions' | ||
| import * as tc from '@actions/tool-cache' | ||
|
|
||
| // TODO: Support version patterns of '2.19.x' to mean the latest 2.19 version. |
There was a problem hiding this comment.
Maybe instead we should support patterns similar to lower bounds in packages?
| import * as versions from './versions' | ||
| import * as tc from '@actions/tool-cache' | ||
|
|
||
| // TODO: Support version patterns of '2.19.x' to mean the latest 2.19 version. |
There was a problem hiding this comment.
I believe mono_repo has a feature to generate tests using the lower bound of the package, maybe setup-dart could also have that feature?
version = auto -> find pubspec yaml (or perhaps lock) and use the lower bound.
| export async function latestPublishedVersion( | ||
| channel: string, | ||
| flavor: string | ||
| ): Promise<string | null> { |
There was a problem hiding this comment.
The null isn't handled at the call sites, shouldn't we throw instead?
Right now, the action will print "Installing the linux-x64 Dart SDK version null..." and then fail somewhere later (probably when it tries to download from dart-archive, which hopefully doesn't have a file at the URL containing null).
Gotcha; I'll respond to Alex's feedback, but will hold off on landing until we've had a chance to talk. |
|
Converting this PR to a draft in favor of #77. |
Convert the implementation of the action to TypeScript:
dart.ymlCI the same, so we could verify correct behavior of the portdart-version1.5.0-dev