Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Z/cli/feature/registry suggestions #60

Conversation

iNewLegend
Copy link
Member

@iNewLegend iNewLegend commented Jun 6, 2024

PR Type

Enhancement, Bug fix


Description

  • Renamed zWorkspaceFindRootPackageJson to zWorkspaceFindRootPackageJsonPath and updated references.
  • Added support for multiple active registries in publish and registry commands.
  • Introduced new commands for listing and cleaning registries.
  • Enhanced ConsoleMenu to include selected item index.
  • Added utility functions for network port management.
  • Updated default registry host and port range.
  • Improved workspace package finding logic and added root package name retrieval.
  • Updated package.json files to include TypeScript config files.

Changes walkthrough 📝

Relevant files
Enhancement
15 files
command-base.ts
Update workspace path and name initialization in CommandBase.

packages/zenflux-cli/src/base/command-base.ts

  • Renamed zWorkspaceFindRootPackageJson to
    zWorkspaceFindRootPackageJsonPath.
  • Added zWorkspaceGetRootPackageName function.
  • Updated initPathsArgs to include workspaceName.
  • +4/-2     
    publish.ts
    Enhance publish command to support multiple registries.   

    packages/zenflux-cli/src/commands/publish.ts

  • Added support for multiple active registries.
  • Updated local registry usage logic.
  • Modified path for prepublish directory.
  • +23/-21 
    registry.ts
    Add multi-registry support and new commands in Registry. 

    packages/zenflux-cli/src/commands/registry.ts

  • Added commands for listing and cleaning registries.
  • Ensured Verdaccio server is not already running before starting.
  • Updated registry usage logic.
  • +117/-21
    global.ts
    Enhance global paths initialization with workspaceEtc.     

    packages/zenflux-cli/src/core/global.ts

  • Added workspaceEtc path.
  • Updated zGlobalInitPaths to use workspaceEtc.
  • +11/-5   
    registry.ts
    Add registry handling functions in core registry module. 

    packages/zenflux-cli/src/core/registry.ts

  • Added functions to handle npm registry configurations.
  • Implemented functions to get all registries and online registries.
  • +71/-0   
    workspace.ts
    Update workspace functions and add root package name retrieval.

    packages/zenflux-cli/src/core/workspace.ts

  • Renamed zWorkspaceFindRootPackageJson to
    zWorkspaceFindRootPackageJsonPath.
  • Added zWorkspaceGetRootPackageName function.
  • Updated workspace package finding logic.
  • +17/-10 
    zenflux.ts
    Update default registry host and port range.                         

    packages/zenflux-cli/src/definitions/zenflux.ts

    • Updated default registry host and port range.
    +2/-2     
    console-menu.ts
    Enhance ConsoleMenu to include selected item index.           

    packages/zenflux-cli/src/modules/console/console-menu.ts

    • Added index to ConsoleMenu selected item.
    +5/-2     
    net.ts
    Add network utility functions for port management.             

    packages/zenflux-cli/src/utils/net.ts

  • Added utility functions for checking port availability and finding
    free ports.
  • +46/-0   
    index.ts
    Update workspace root package JSON path function in zenflux-jest.

    packages/zenflux-jest/src/index.ts

  • Renamed zWorkspaceFindRootPackageJson to
    zWorkspaceFindRootPackageJsonPath.
  • +2/-2     
    utils.d.ts
    Enhance getMatchingPathsRecursive with options parameter.

    packages/zenflux-typescript-vm/types/utils.d.ts

    • Updated getMatchingPathsRecursive to include options parameter.
    +6/-1     
    workspace-provider.js
    Refactor workspace provider for improved package directory finding.

    packages/zenflux-typescript-vm/src/providers/workspace-provider.js

  • Refactored workspace provider to use new package directory finding
    logic.
  • +26/-14 
    utils.js
    Enhance getMatchingPathsRecursive with options parameter.

    packages/zenflux-typescript-vm/src/utils.js

    • Updated getMatchingPathsRecursive to include options parameter.
    +15/-9   
    package.json
    Update package.json to include TypeScript config files.   

    packages/zenflux-cli/package.json

    • Added tsconfig.json and tsconfig.paths.json to files.
    +3/-1     
    package.json
    Update package.json to include TypeScript config files.   

    packages/zenflux-typescript-vm/package.json

    • Added tsconfig.json and tsconfig.import-meta.json to files.
    +3/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    …ckage.json` using new `getMatchingPathRecursive`
    
    Renamed `zWorkspaceFindRootPackageJson` to `zWorkspaceFindRootPackageJsonPath` and used it in various parts of `zenflux-jest` and `zenflux-cli` code. Optimized workspace functions to better filter packages containing `package.json` using improved path fetching method. Added the function `zWorkspaceGetRootPackageName` in `workspace.ts`, which was then implemented in `command-base.ts`.
    … support multi registries
    
    The registry handling has been updated to support multiple active registries. This includes the addition of `@list` and `@clean` commands. The Verdaccio server-running command now ensures it is not already running before starting, and locates a free port within the default range. The `@use` command is updated to allow for selection of the registry by ID.
    @codiumai-pr-agent-free codiumai-pr-agent-free bot added enhancement New feature or request Bug fix labels Jun 6, 2024
    @iNewLegend iNewLegend changed the base branch from z/cli/feature/registry-suggestions to main June 6, 2024 16:19
    @iNewLegend iNewLegend changed the base branch from main to z/cli/feature/registry-suggestions June 6, 2024 16:19
    @iNewLegend iNewLegend merged commit 4eccd79 into ZenFlux:z/cli/feature/registry-suggestions Jun 6, 2024
    3 of 4 checks passed
    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    4, because the PR introduces a significant amount of changes across multiple files and functionalities, including network operations, command line interfaces, and configuration management. The complexity of the changes, especially with the addition of new network utility functions and modifications to registry handling, requires careful review to ensure functionality and security.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The use of zNetCheckPortOnline with a very short timeout (10 ms) in packages/zenflux-cli/src/utils/net.ts might lead to false negatives in checking port availability due to such a brief wait period.

    Error Handling: In packages/zenflux-cli/src/core/registry.ts, the function zRegistryGetNpmRc throws an error if the token cannot be found in the .npmrc file. This could be handled more gracefully to avoid crashing the application.

    Performance Concern: The recursive directory search in packages/zenflux-typescript-vm/src/utils.js could potentially lead to performance issues if the directory structure is very large or deep.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for network requests

    Use a try-catch block around the fetch call to handle potential network errors gracefully.

    packages/zenflux-cli/src/commands/registry.ts [63-66]

    -const response = await fetch( `${ url }/-/user/org.couchdb.user:${ DEFAULT_Z_REGISTRY_USER }`, {
    -    method: "PUT",
    -    headers: {
    -        "Accept": "application/json",
    +try {
    +    const response = await fetch( `${ url }/-/user/org.couchdb.user:${ DEFAULT_Z_REGISTRY_USER }`, {
    +        method: "PUT",
    +        headers: {
    +            "Accept": "application/json",
    +    });
    +} catch (error) {
    +    ConsoleManager.$.error(`Failed to fetch user data: ${error}`);
    +    return;
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding a try-catch block around the fetch call to handle potential network errors gracefully addresses a possible bug and improves the robustness of the code. This is a significant improvement.

    9
    Add error handling to file reading and processing in zRegistryGetNpmRc

    Refactor the zRegistryGetNpmRc function to handle potential exceptions from
    fs.readFileSync and crypto.createHash more gracefully by using try-catch blocks.

    packages/zenflux-cli/src/core/registry.ts [16-17]

    -const content = fs.readFileSync( path, "utf8" ),
    +let content, match;
    +try {
    +    content = fs.readFileSync( path, "utf8" );
         match = content.match( /\/\/(.*):(\d+)\/:_authToken=(.*)/ );
    +} catch (error) {
    +    console.error(`Error reading or processing ${path}:`, error);
    +    return null;
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling to the zRegistryGetNpmRc function is a valuable improvement that increases the robustness of the code by preventing unhandled exceptions.

    8
    Enhancement
    Enhance socket management by clearing the timeout after a connection or error

    Modify the zNetCheckPortOnline function to handle the socket timeout more robustly by
    clearing the timeout after a connection is established or an error occurs, to prevent
    potential memory leaks or unexpected behavior.

    packages/zenflux-cli/src/utils/net.ts [14-20]

    -setTimeout( timeout, 10 );
    +const timeoutId = setTimeout( timeout, 10 );
    +socket.on( "connect", () => {
    +    clearTimeout(timeoutId);
    +    resolve( true );
    +    socket.destroy();
    +});
     
    Suggestion importance[1-10]: 9

    Why: This suggestion enhances the robustness of the zNetCheckPortOnline function by ensuring the timeout is cleared, which prevents potential memory leaks and unexpected behavior. This is a significant improvement.

    9
    Performance
    Use asynchronous file operations to improve performance

    Replace the synchronous file operations with asynchronous ones to improve performance and
    avoid blocking the event loop.

    packages/zenflux-cli/src/commands/registry.ts [79]

    -fs.writeFileSync( paths.npmRc, data );
    +await fs.promises.writeFile( paths.npmRc, data );
     
    Suggestion importance[1-10]: 8

    Why: Replacing synchronous file operations with asynchronous ones can improve performance and avoid blocking the event loop, which is beneficial for the application's responsiveness. This is a valuable enhancement.

    8
    Maintainability
    Refactor server shutdown logic into a separate function

    Extract the logic for server shutdown into a separate function to improve code readability
    and reusability.

    packages/zenflux-cli/src/commands/registry.ts [83-88]

    -process.on( "SIGINT", () => {
    +process.on( "SIGINT", shutdownServer );
    +
    +function shutdownServer() {
         ConsoleManager.$.log( "Shutting down server..." );
         server.close( () => {
             ConsoleManager.$.log( "Server closed" );
             process.exit( 0 );
         });
    -});
    +}
     
    Suggestion importance[1-10]: 7

    Why: Extracting the server shutdown logic into a separate function improves code readability and reusability. This is a good practice for maintainability, though it is not critical.

    7
    Centralize URL formatting for logging

    Replace the direct use of util.inspect with a dedicated function to format URLs for
    logging. This will centralize URL formatting and make it easier to adjust the formatting
    in one place if needed.

    packages/zenflux-cli/src/commands/registry.ts [55]

    -ConsoleManager.$.log( `You can access the registry at ${ util.inspect( url ) }` );
    +ConsoleManager.$.log( `You can access the registry at ${ formatUrlForLogging(url) }` );
     
    Suggestion importance[1-10]: 6

    Why: The suggestion to centralize URL formatting for logging improves maintainability by making it easier to adjust the formatting in one place if needed. However, this change is not crucial and only brings a small improvement.

    6
    Possible issue
    Ensure path resolutions are dependent on the successful determination of etcPath

    In the zGlobalInitPaths function, ensure that workspaceEtcPath and verdaccioPath are only
    resolved if etcPath is successfully determined to avoid potential errors from undefined
    paths.

    packages/zenflux-cli/src/core/global.ts [57-59]

    -const etcPath = path.resolve( os.homedir(), DEFAULT_Z_ETC_FOLDER ),
    -    workspaceEtcPath = path.resolve( etcPath, args.workspaceName ),
    +const etcPath = path.resolve( os.homedir(), DEFAULT_Z_ETC_FOLDER );
    +let workspaceEtcPath, verdaccioPath;
    +if (etcPath) {
    +    workspaceEtcPath = path.resolve( etcPath, args.workspaceName );
         verdaccioPath = path.resolve( workspaceEtcPath, DEFAULT_Z_VERDACCIO_FOLDER );
    +}
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the reliability of the zGlobalInitPaths function by ensuring dependent paths are only resolved if the base path is successfully determined. This is a useful enhancement for preventing potential runtime errors.

    7
    Improve type safety by allowing workspaceName to be undefined

    Consider using a more specific type for the workspaceName parameter in the
    zGlobalInitPaths function to ensure it is always a string and not potentially undefined.
    This can help avoid runtime errors when resolving paths.

    packages/zenflux-cli/src/core/global.ts [53]

    -workspaceName: string,
    +workspaceName: string | undefined,
     
    Suggestion importance[1-10]: 3

    Why: The suggestion to allow workspaceName to be undefined is not necessary since the current implementation assumes it is always a string. Allowing it to be undefined could introduce more complexity without significant benefit.

    3

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant