DOP-4209 #958
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,16 +43,25 @@ async function createEnvProdFile({ | |
| } | ||
| } | ||
|
|
||
| export async function downloadBuildDependencies( | ||
| buildDependencies: BuildDependencies, | ||
| repoName: string, | ||
| directory?: string | ||
| ) { | ||
| const commands: string[] = []; | ||
| await Promise.all( | ||
| buildDependencies.map(async (dependencyInfo) => { | ||
| const repoDir = directory ? getRepoDir(repoName, directory) : getRepoDir(repoName); | ||
|
There was a problem hiding this comment. No need to do this conditional! The
|
||
| const targetDir = dependencyInfo.targetDir ?? repoDir; | ||
| let options = {}; | ||
| if (targetDir != repoDir) { | ||
| options = { cwd: repoDir }; | ||
| } | ||
| try { | ||
| await executeCliCommand({ | ||
| command: 'mkdir', | ||
| args: ['-p', targetDir], | ||
| options: options, | ||
| }); | ||
| } catch (error) { | ||
| console.error( | ||
|
|
@@ -63,17 +72,19 @@ export async function downloadBuildDependencies(buildDependencies: BuildDependen | |
| } | ||
| commands.push(`mkdir -p ${targetDir}`); | ||
| await Promise.all( | ||
|
There was a problem hiding this comment. Note: I did not use There was a problem hiding this comment. What version of node are you using? |
||
| dependencyInfo.dependencies.map(async (dep) => { | ||
| try { | ||
| executeCliCommand({ | ||
| command: 'curl', | ||
| args: ['--max-time', '10', '-SfL', dep.url, '-o', `${targetDir}/${dep.filename}`], | ||
| options: options, | ||
| }); | ||
| } catch (error) { | ||
| console.error( | ||
| `ERROR! Could not curl ${dep.url} into ${targetDir}/${dep.filename}. Dependency information: `, | ||
| dependencyInfo | ||
| ); | ||
| return commands; | ||
| } | ||
| commands.push(`curl -SfL ${dep.url} -o ${targetDir}/${dep.filename}`); | ||
| }) | ||
|
There was a problem hiding this comment. Hmm, interesting thing here. I could be wrong, but I believe that because we're returning a static value (even though it's a Promise because of the async keyword) it can return before the I think this can be slightly changed to do what you're trying to do and cleaner too. If you take away the I have an example below. Like this: There was a problem hiding this comment. These are all great points, @mmeigs. The one small nit with the example though is that we won't hit this catch block, I think. This is because we return the promise, but we don't await it, not giving the promise the opportunity to settle before the catch block can be entered. I think we can add the This also has me thinking as well, do we want to do an There was a problem hiding this comment. Testing this out with some incorrect curl links but this approach still doesn't seem to catch the error 😞 There was a problem hiding this comment. Hmm, I wonder if doing const cliResult = await executeCliCommand(...);
return cliResult;in the try/catch? I wonder if return is sort of overwriting the There was a problem hiding this comment. this didn't work either 💔 i dropped a new ticket for this issue though! |
||
|
|
@@ -87,12 +98,9 @@ export async function prepareBuildAndGetDependencies( | |
| repoName: string, | ||
| projectName: string, | ||
| baseUrl: string, | ||
| directory?: string | ||
|
There was a problem hiding this comment. If we're not getting dependencies in this function anymore, can we rename this function? |
||
| ) { | ||
| const repoDir = getRepoDir(repoName, directory); | ||
|
|
||
| // doing these in parallel | ||
| const commandPromises = [ | ||
|
|
||
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.
nit: Make sure the merge finishes and gets rid of these characters
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.
Regarding the diff: The quotes are not necessary, but they are also not an issue. Either way is fine, but I would lean toward all the examples showing the same way for each input
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.
Just noting here that this entire file has multiple unfinished merge flags to take care of!