Conversation
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
bot
removed
🧩 core
🧩 js-packages-plugin
…/utils/command-helper # Conflicts: # packages/nx-plugin/src/executors/cli/executor.int.test.ts # packages/nx-plugin/src/internal/execute-process.ts # packages/utils/src/lib/execute-process.ts
| const { | ||
| command, | ||
| args, | ||
| observer, |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
matejchalk
left a comment
matejchalk
left a comment
There was a problem hiding this comment.
The more I see in this PR, the more confused I am about what it solves.
- De-duplicating
executeProcess? No, I did that already in #1137. - De-duplicating helper functions like
objectToCliArgs? No, in fact, this PR makes it even worse. It adds duplicate implementations and tests of many helper functions inpackages/utils/src/lib/command.ts, but they aren't imported anywhere. So functions likeobjectToCliArgsare now duplicated in 3 places, not 2. And there's no reason for it.- There's a much simpler solution to avoid duplicating the helper functions, which is to dynamically import
@code-pushup/utils. From what I saw when I un-duplicatedexecuteProcess, there really isn't that much code being used anyway. I'm happy to create a small PR that removes all the duplicates, so we can close this topic.
- There's a much simpler solution to avoid duplicating the helper functions, which is to dynamically import
In the interest of saving time, I would abandon this PR. I don't think it solves anything anymore.
| /** | ||
| * Dynamically imports and executes function from utils. | ||
| * | ||
| * This is a workaround for Nx only supporting plugins in CommonJS format. | ||
| */ | ||
| export async function executeProcess( | ||
| cfg: import('@code-pushup/utils').ProcessConfig, | ||
| ): Promise<import('@code-pushup/utils').ProcessResult> { | ||
| const { executeProcess } = await import('@code-pushup/utils'); | ||
| return executeProcess(cfg); | ||
| } |
There was a problem hiding this comment.
Was it intentional to delete this wrapper? I created it to simplify usage within the nx-plugin.
| const { executeProcess } = await import('@code-pushup/utils'); | ||
| await executeProcess({ |
There was a problem hiding this comment.
The dynamic import wouldn't be needed here if you imported executeProcess from the packages/nx-plugin/src/internal/execute-process.ts you deleted (see previous comment).
It's up to you, though. I don't really mind either way as long as the implementation is being dynamically imported. The local wrapper is just for convenience.
| if (isVerbose() || verbose === true) { | ||
| loggerInstance.info( | ||
| formatCommandLog({ | ||
| command, | ||
| args, | ||
| cwd: cfg.cwd ? String(cfg.cwd) : process.cwd(), | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Please revert changes to this file. They're not compatible with the work I did in #1137.
- The logger now handles the verbose flag internally.
- The
logger.commandmethod below already prints the command and args, so this would log a duplicate. - An option to pass custom logger instances introduces unnecessary complexity, and we don't need it for anything.
| export function formatCommandLog(options: FormatCommandLogOptions): string { | ||
| const { command, args = [], cwd = process.cwd(), env } = options; | ||
| const relativeDir = path.relative(process.cwd(), cwd); | ||
|
|
||
| return [ | ||
| ...(relativeDir && relativeDir !== '.' | ||
| ? [ansis.italic(ansis.gray(relativeDir))] | ||
| : []), | ||
| ansis.yellow('$'), | ||
| ...(env && Object.keys(env).length > 0 | ||
| ? Object.entries(env).map(([key, value]) => { | ||
| return ansis.gray(`${key}=${formatEnvValue(value)}`); | ||
| }) | ||
| : []), | ||
| ansis.gray(command), | ||
| ansis.gray(args.join(' ')), | ||
| ].join(' '); | ||
| } |
There was a problem hiding this comment.
This function was integrated into the new logger, and I removed this helper in #1137.
There was a problem hiding this comment.
All these functions and tests are now duplicated. And I don't understand why. The command.ts module isn't being used anywhere.
| 'error', | ||
| { ignoredDependencies: ['esbuild'] }, // esbuild is a peer dependency of bundle-require | ||
| { | ||
| ignoredDependencies: ['esbuild', 'ora'], // esbuild is a peer dependency of bundle-require, ora has transitive dependencies with different versions |
There was a problem hiding this comment.
The ora package is a direct dependency of @code-pushup/utils, so there's no need to ignore anything. The linter passes in the main branch, so I don't see what change is needed.
|
The goal of this pr was in fact to dedupe and move code in one place. As it was long hanging due to feedback and travel overlap other PR's (you mentioned yours for example) got merged. I did merged main and must have missed these changes your PR introduced and other duplicates. I will give it a cleanup as I still see it helpful to have all command related things in 1 place/file. Thanks for your patience and pointing out that you could help with a PR that includes the above changes to save time. I feel I'm more motivated to finish the implementation my self, so I will try it another time. I will share PR descriptions first. To reduce feedback and discussion:
Update: I'll close and reopen new one after I'm steady available again. |
2 participants
This PR includes:
command.tsexecuteProgress- removed hard dependencies to other functions so it is easier to use innx-pluginimportforexecuteProcess.tsinnx-plugin