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

Add package manager selection and --dry-run option #92

Draft
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

outslept
Copy link

@outslept outslept commented Dec 27, 2024

This PR introduces a significant overhaul to the npm init stylelint command, focusing on providing users with greater control and transparency during the initial setup process. I've implemented two key features:

  1. Interactive package manager selection and
  2. A --dry-run option.

Motivation

The existing npm init stylelint command, while functional, makes implicit assumptions that can lead to friction and confusion for users:

  • Implicit npm Assumption: The tool automatically uses npm for dependency installation, disregarding the user's potential preference or existing project setup with pnpm, yarn, or bun. This forces manual intervention and can lead to inconsistencies in dependency management.

  • Lack of Transparency and Safety: The tool directly modifies the file system without providing a way to preview the intended actions. This can be problematic for users who want to understand the changes before they are applied or for debugging potential issues.

This PR directly addresses these shortcomings, resulting in a more observable initialization experience.

Breakdown of the changes Made:

1. src/actions/context.ts:

  • Purpose: This file is responsible for establishing the execution context for the npm init stylelint command. It parses command-line arguments, detects the package manager, and creates a Context object that holds information, which we need throughout the initialization process.

2. src/actions/create-config.ts:

  • Purpose: This file handles the creation of the .stylelintrc.json configuration file.

3. src/actions/ensure-package-json.ts:

  • Purpose: This file ensures that a package.json file exists in the project directory. If it doesn't, it attempts to create one.

4. src/actions/help.ts:

  • Purpose: This file contains the logic for displaying the help information when the --help flag is used.

5. src/actions/install.ts:

  • Purpose: This file handles the installation of the necessary Stylelint dependencies.

6. src/actions/post-install.ts:

  • Purpose: This file is responsible for displaying the post-installation message to the user, providing guidance on how to use Stylelint.

7. src/prompts/install-now.ts:

  • Purpose: This file handles the prompt asking the user if they want to install the dependencies now.

8. src/prompts/package-manager.ts:

  • Purpose: This file implements the interactive prompt for selecting the preferred package manager.

9. src/prompts/usage-preference.ts:

  • Purpose: This file handles the prompt asking the user about their preferred Stylelint usage (catching errors only or enforcing style).

10. src/shell.ts:

  • Purpose: This file provides a lightweight abstraction over the child_process.spawn function for executing shell commands.

Note

This was created to replace execa, reducing the project's dependency footprint. Comes from astro - https://github.com/withastro/astro/blob/main/packages/upgrade/src/shell.ts

11. index.ts:

  • Purpose: This is the main entry point of the npm init stylelint command. It orchestrates the entire initialization process.

Related Issues

#16
#91

This commit introduces a new, self-contained module located at `src/actions/create-config.ts`. This module is designed to handle creating and managing the Stylelint configuration file.

**Functionality:**

- **Configuration Existence Check:** The module begins by implementing a mechanism for detecting existing Stylelint configurations. This is crucial to prevent accidental overwriting of user-defined settings. The `findExistingConfig` function leverages the `cosmiconfigSync` utility.  This search ensures that all common configuration locations are considered. This prevents accidental overwrites.

- **Conditional Configuration Creation:** Based on the user's initial preference (selected in a prior step), the module determines the appropriate base configuration to extend. If the user opted for a basic setup focused on error detection, `stylelint-config-recommended` is chosen. For users desiring a more opinionated and comprehensive setup, `stylelint-config-standard` is selected.
This commit introduces the `src/actions/ensure-package-json.ts` module to ensure a `package.json` file exists in the project directory.

Key changes:

- Implements `hasPackageJson` which uses `fs.readdirSync` to check if `package.json` exists in the directory.
- Implements `initializePackageJson` which creates a default `package.json` if one is missing. It uses `execa` to run the `npm init -y` command.
- Implements `ensureProjectPackageJson` as the main function to check for and potentially create the `package.json`.
This commit introduces the `installProjectDependencies` action to handle the installation of necessary project dependencies.

Key changes:

- Implements `installProjectDependencies` which uses `execa` to execute the package manager's install command.
- Installs `stylelint` and either `stylelint-config-recommended` or `stylelint-config-standard` based on the user's usage preference.
- Uses `getInstallCommand` to determine the correct install command (`install` for npm, `add` for others).
This commit introduces the `promptInstallDependencies` function to ask the user whether to install specified dependencies.

Key changes:

- Uses the `prompts` library to present a confirmation prompt to the user.
- The prompt message includes the package manager and the list of dependencies to be installed.
- Returns a boolean indicating the user's choice.
This commit introduces the `promptPackageManager` function to allow the user to select their preferred package manager (npm, pnpm, or yarn).

Key changes:

- Uses the `prompts` library to present a selection prompt.
- Uses `which-pm-runs` to pre-select the package manager if it's already being used in the project.
- Returns the selected package manager.
This commit introduces the `promptUsagePreference` function to let the user choose their preferred Stylelint usage level (error detection only or error detection and style enforcement).
This commit introduces a `messages` module to centralize all user-facing messages within the application.
This commit introduces a signal handler to gracefully handle the SIGINT signal (typically triggered by Ctrl+C).

Key changes:

- Implements `setupProcessHandlers` to attach a listener to the `SIGINT` signal.
- When the signal is received, the handler logs a message indicating cancellation and then exits the process.
This commit introduces the main application logic within the `main` function.

Key changes:

- Sets up process handlers for graceful cancellation.
- Checks for existing Stylelint configurations and exits if one is found.
- Ensures the existence of a `package.json` file.
- Prompts the user for their preferred Stylelint usage (errors only or errors and style).
- Prompts the user to select their preferred package manager.
- Prompts the user to confirm the installation of necessary dependencies.
- Conditionally installs dependencies and creates the Stylelint configuration file based on user input.
- Displays next steps and customization information upon successful setup.
…e manager support

Updates the package description to be more generic ("The initialization script for Stylelint.") instead of being specific to npm init.

Also updates the keywords to better reflect the functionality and improve discoverability, including terms like "configuration", "initializer", "cli", and "setup".
…ed dependencies, added context, minor fixes to improve readability
@outslept outslept changed the title CLI Rework PM Selection and --dry-run option for stylelint init (BREAKING) Dec 27, 2024
@outslept
Copy link
Author

outslept commented Dec 27, 2024

I have some questions to discuss before we ship this.

  1. Currently, when a user presses Ctrl + C while an interactive prompt (e.g., package manager selection, install dependencies confirmation) is active, the initialization process doesn't terminate immediately. Instead, it seems to proceed to the next prompt in the sequence. What do we think is the expectable behavior here?
  2. How do we approach testing with this changes?

If you have any questions or suggestions for improvements, please @mention me in here. I'm currently having some health issues, so being directly mentioned will help me track notifications better. I will check this draft from time to time, when we're ready with the suggestions I'll begin implementing new tests and an entry point in the root folder (index.mjs).

@outslept
Copy link
Author

Example workflow with package.json already existing (no audio):

2024-12-27.16-36-53.mp4

@outslept
Copy link
Author

Example workflow with no package.json existing (no audio):

2024-12-27.16-42-11.mp4

@outslept
Copy link
Author

@jeddy3 Thank you for your feedback! It's currently 8 PM my time, and I'm just about to join my family to celebrate New Year's.

As for your question, in short, I noticed they test individual 'modules' using fixtures and that's the play. I also have some tests already that haven't been submitted yet.

I'll be back with more detailed comments and information tomorrow afternoon, after the holiday celebration.

Wishing you and your family a very Happy New Year filled with joy, success, and good health! 🎉✨

@jeddy3
Copy link
Member

jeddy3 commented Dec 31, 2024

It's great that you're willing to post some more details tomorrow. Please don't feel rushed, though. As open-source contributors, we do what we can when we can.

Thank you for your kind words about the new year. I wish you and yours all the best in 2025! 🎉

@outslept
Copy link
Author

outslept commented Jan 1, 2025

Latest Updates TLDR (mostly UX)

Preview of the latest changes (audio muted, video is speeded up)

2025-01-01.15-26-28.mp4

Questions UX related

I've been critically examining our CLI's user experience, comparing it with Astro's approach. While our current implementation provides functionality, there are opportunities for improvement in user guidance and interaction clarity.

Add package command Packages install command
Create a file command Create Tailwind Config
File Contents File Contents Tailwind Config

So the questions are:

  1. Should we introduce more verbose, guided installation steps? Like prompt to create stylelint config file, show file contents of the stylelint config file, prompt to save it and so on?
  2. Can or should we provide more contextual information during configuration? About the commands to be ran like on 1st screenshot and etc.

This looks nice, isn't hard to implement, yet I'm overwhelmed so that the tool should save the balance between being informative, but not overly verbose. I want the implemented functionality to be good and intuitive for the dev either if he is super skilled or not at all.

Testing

I assume that our approach is as follows:

└── 📁test
    └── 📁fixtures
        └── 📁configs
            └── stylelint.config.js
            └── stylelint.config.mjs
            └── stylelint.config.cjs
            └── .stylelintrc.js
            └── .stylelintrc.mjs
            └── .stylelintrc.cjs
            └── .stylelintrc.json
            └── .stylelintrc.yml
            └── .stylelintrc.yaml
            └── .stylelintrc
            └── package.json
        └── 📁package-json
            └── no-package.json
            └── valid-package.json
    └── 📁actions
         └── context.test.ts
         └── create-config.test.ts
         └── help.test.ts
         └── install.test.ts
         └── post-install.test.ts
    └── 📁prompts
         └── install-now.test.ts
         └── package-manager.test.ts
    └── 📁utils
         └── helpers.test.ts
         └── isOnline.test.ts
         └── isWriteable.test.ts
         └── registry.test.ts
    └── integration.test.ts

I think it would be appropriate to fixtures like this:

const configs = {
  js: './fixtures/configs/stylelint.config.js',
  mjs: './fixtures/configs/stylelint.config.mjs',
  // and so one
};

As long as all this formats are in docs I think they need to be tested.

Modules as I wrote above should be tested in isolation and itntegration test will check the full flow of CLI operation.

Currently actions tests are finished. Prompts, utils and main flow to be done. Planning on getting PR out of the draft when I'm done with the tests.

@jeddy3
Copy link
Member

jeddy3 commented Jan 2, 2025

Thank you for your latest additions. It's shaping up to be a useful and robust tool!

Should we introduce more verbose, guided installation steps? Can or should we provide more contextual information during configuration

Yes, I believe both improve the user experience as people can:

  • see exactly what the script will do
  • interactively step through each part

What do you think about the following?:

jeddy3@mac create-stylelint % npm start         

> [email protected] start
> node create-stylelint.mjs

✔ Which package manager do you want to use? › npm
✔ This tool will install dependencies using the following command:

  npm install stylelint@~16.12.0 stylelint-config-standard@~36.0.1

Continue? … yes
✔ Installed dependencies.
✔ This tool will create a `stylelint.config.mjs` file with the following content:

  export default {
    extends: ["stylelint-config-standard"]
  };

Continue? … yes
✔ Created config file.

Success! You can now lint your CSS files by running the command:

  npx stylelint "**/*.css"

Please refer to the official Stylelint documentation for more customization options:
https://stylelint.io/user-guide/configure/

For --dry-run, it'd be great if we had the same behaviour as Astro, i.e. being able to step through the process:

> [email protected] start --dry-run
> node create-stylelint.mjs

Dry run mode.

✔ Which package manager do you want to use? › npm
✔ This tool will install dependencies using the following command:

  npm install stylelint@~16.12.0 stylelint-config-standard@~36.0.1

Continue? … yes
i --dry-run Skipping installation
✔ This tool will create a `stylelint.config.mjs` file with the following content:

  export default {
    extends: ["stylelint-config-standard"]
  };

Continue? … yes
i --dry-run Skipping file creation

Dry run complete! 

Do you think that'd be possible? (Adding details to each step makes the --dry-run flag much more useful.)

@outslept
Copy link
Author

Hey. I wanted to apologize for the delay in getting this PR finalized. I’ve been working on it in my spare time, and I’ve run into a couple of issues with memfs for mocking the fs. These issues slowed things down a bit for me. I’ll provide an update as soon as I’ve resolved the remaining problems. Thanks for your patience, and I appreciate your understanding while I wrap this up!

@jeddy3
Copy link
Member

jeddy3 commented Jan 13, 2025

@gnify No need to apologise. As open-source contributors, we do what we can when we can. Many thanks for working on this in your spare time. Take as long as you need. We appreciate your efforts. The PR is looking great and significantly improves user experience, especially for people encountering Stylelint for the first time. So, thank you!

@Mouvedia
Copy link
Member

Mouvedia commented Feb 7, 2025

…I’ve run into a couple of issues with memfs for mocking the fs.

@outslept would it be easier for you if the PR was split in several PR?
i.e. skip the parts that require those tests for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion
medium

I think you can achieve what you want using boxen.
e.g. boxen('foo', { padding: 1, title: 'my-file', titleAlignment: 'left', borderStyle: 'round' })

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm intentionally avoiding boxen because it's too "heavyweight" for this task. It pulls in many dependencies, while my approach is to use something more lightweight and maybe easier to maintain? I couldn't find anything lighter on npm, so I've implemented my own.

Additionally, some of boxen's deps can now be replaced with native Node.js functionality, for example stripVTControlCharacters is a drop-in replacement for strip-ansi. This should help having the same functionality with less footprint while having the functionality needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal was to lower the number of files to review and maintain.
It's a matter of priorities. You might be right in thinking that it is more efficient to have a bespoke solution.
I don't know.

@jeddy3 what do you think?

@Mouvedia
Copy link
Member

Once you want reviews on your PR remember to mark it as ready.

'n/no-process-exit': 'off',
"n/no-unsupported-features/node-builtins": ["error", {
"version": ">=18.12.0",
"ignores": ["fetch"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question

Maybe you want allowExperimental: true instead of ignoring fetch?

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

Successfully merging this pull request may close these issues.

3 participants