Add factory #7
Add factory #7
Conversation
| protected filterVersions(nodeVersions: INodeVersion[]) { | ||
| const versions: string[] = []; | ||
|
|
||
| const dataFileName = this.getDistFileName(this.nodeInfo.arch); |
There was a problem hiding this comment.
nitpick: I think we can get the architecture directly in the method since nodeInfo is available as a class property
| const evaluatedVersion = this.evaluateVersions(versions); | ||
|
|
||
| if (evaluatedVersion) { | ||
| this.nodeInfo.versionSpec = evaluatedVersion; |
There was a problem hiding this comment.
I don't like the idea of overwriting the versionSpec property since we need to keep in mind that it changes at some point in time, this is not obvious. In my understanding, this violates the principle of encapsulation. I prefer to use a separate class property for this case.
| toolPath = await this.downloadNodejs(toolName); | ||
| } | ||
|
|
||
| if (this.osPlat != 'win32') { |
There was a problem hiding this comment.
We use such checks in several places. Should we add the isWindows variable just for readability?
| `Unable to find Node version '${this.nodeInfo.versionSpec}' for platform ${this.osPlat} and architecture ${this.nodeInfo.arch}.` | ||
| ); | ||
| } | ||
| const toolName = this.getNodejsDistInfo(evaluatedVersion, this.osPlat); |
There was a problem hiding this comment.
nitpick: I think we can get the osPlat directly in the method since it is available as a class property.
I am also confused by the variable name (toolName). We get a complex object, not just the tool name.
| } | ||
| } | ||
|
|
||
| let toolPath = this.findVersionInHoostedToolCacheDirectory(); |
src/distibutions/base-models.ts
Outdated
| @@ -0,0 +1,18 @@ | |||
| export interface INodejs { | |||
There was a problem hiding this comment.
May be rename to something like Inputs or ActionInputs or NodeInputs?
| function identifyDistribution(versionSpec: string) { | ||
| let distribution = Distributions.DEFAULT; | ||
| if (versionSpec.includes(Distributions.NIGHTLY)) { | ||
| distribution = Distributions.NIGHTLY; | ||
| } else if (versionSpec.includes(Distributions.CANARY)) { | ||
| distribution = Distributions.CANARY; | ||
| } else if (versionSpec.includes(Distributions.RC)) { | ||
| distribution = Distributions.RC; | ||
| } | ||
|
|
||
| return distribution; | ||
| } | ||
|
|
||
| export function getNodejsDistribution( | ||
| installerOptions: INodejs | ||
| ): BaseDistribution { | ||
| const distributionName = identifyDistribution(installerOptions.versionSpec); | ||
| switch (distributionName) { | ||
| case Distributions.NIGHTLY: | ||
| return new NightlyNodejs(installerOptions); | ||
| case Distributions.CANARY: | ||
| return new CanaryBuild(installerOptions); | ||
| case Distributions.RC: | ||
| return new RcBuild(installerOptions); | ||
| default: | ||
| return new OfficialBuilds(installerOptions); | ||
| } |
There was a problem hiding this comment.
May be we can merge it to the single method? This way we would avoid duplication.
| nodeVersions = nodeVersions ?? (await this.getNodejsVersions()); | ||
| const versions = this.filterVersions(nodeVersions); | ||
| const evaluatedVersion = this.evaluateVersions(versions); |
There was a problem hiding this comment.
these 3 lines look like duplicate of what we have under checkLatest condition.
What if we include filterVersions inside this.getNodejsVersions?
| super(nodeInfo); | ||
| } | ||
|
|
||
| protected findVersionInHoostedToolCacheDirectory(): string { |
There was a problem hiding this comment.
This method seems to be identical in all subclasses. Can we move it to base class? Or may be simplify it somehow to avoid code duplication.
6 participants
Description:
Describe your changes.
Related issue:
Add link to the related issue.
Check list: