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

[rush] Enable Prettier integration #1573

Closed
octogonz opened this issue Oct 7, 2019 · 10 comments
Closed

[rush] Enable Prettier integration #1573

octogonz opened this issue Oct 7, 2019 · 10 comments
Labels
enhancement The issue is asking for a new feature or design change

Comments

@octogonz
Copy link
Collaborator

octogonz commented Oct 7, 2019

I'm opening this issue to track the plan for replacing TSLint with the new @microsoft/eslint-config-scalable-ts package that uses ESLint+TypeScript+Prettier.

Here's a rough outline of the steps:

  1. (Done) Converted the TSDoc repo to use the new ruleset as an initial pilot test
  2. (Done) A month ago we also converted an internal production repo to use it, along with Prettier.
  3. (In progress) Create a gulp-core-build package for running ESLint (instead of TSLint). This is @iclanton 's PR [GCB-eslint] Create a package for running ESLint in gulp-core-build. #1565
  4. Convert the rushstack repo to use this package when building projects
  5. Enable Prettier in the rushstack repo
  6. Write up some docs for using Prettier in a Rush repo, since this will be a Rush Stack best practice

I've also been wondering if Rush maybe should include built-in integration for Prettier, to avoid the hassle of writing custom Git hooks.

@octogonz
Copy link
Collaborator Author

octogonz commented Oct 7, 2019

For reference the PR #1369 discussion contains some of our rationale behind the decision to adopt Prettier.

@octogonz octogonz added the general discussion Not a bug or enhancement, just a discussion label Oct 7, 2019
@octogonz
Copy link
Collaborator Author

octogonz commented Jun 6, 2020

5. Enable Prettier in the rushstack repo

6. Write up some docs for using Prettier in a Rush repo, since this will be a Rush Stack best practice

Status Update: This weekend I'm finally moving forward with these last couple items.

@iclanton and I had a long chat about the approach for Prettier. We considered generalizing the install-run.js concept to support install-on-demand of an isolated subfolder containing a tool like pretty-quick and prettier and associated scripts. It led to a few Rush feature ideas that would be generally useful.

However, we were concerned this approach will necessarily require more set up steps, and more documentation about how all the pieces work. Since Prettier is the Rush Stack bet for formatting, we decided instead to design around the goal of making Prettier easy to set up and fully integrated. The proposed design is that you simply specify "prettierVersion": "2.0.5" in rush.json, and Rush will handle everything else.

(It will still be possible to not use that setting, and instead roll your own complex rule mappings using lint-staged and so forth. But our insight was that advanced users wanting those advanced features will prefer flexibility versus hand-holding. So the way to support them is via GitHub issues people create as they are trying to implement their advanced configurations.)

@octogonz octogonz changed the title Finish rolling out eslint-config-scalable-ts [rush] Enable Prettier integration Jun 6, 2020
@octogonz octogonz added enhancement The issue is asking for a new feature or design change and removed general discussion Not a bug or enhancement, just a discussion labels Jun 6, 2020
@octogonz
Copy link
Collaborator Author

octogonz commented Jun 7, 2020

Design Goal

Users can simply turn on a setting ("stackPrettierEnabled": true), Rush gives you a recommended and battle-tested setup for using Prettier. (Users who want an advanced custom configuration would probably not need this feature.)

This makes sense as a Rush-level feature because the Prettier commit hook processes a cross-section of files from multiple projects at once. By contrast, I would NOT use this design for ESLint, because we recommend to invoke ESLint as a build task. Thus ESLint's configuration is owned by the toolchain, which can very between Rush projects in a monorepo.

Proposed design

Rush will provide a new config file for managing Prettier integration. Something like this:

common/config/rush/stack-prettier.json

{
  // "Stack" means that the Prettier configuration is managed by Rush and uses the
  // Rush Stack recommended setup.
  //
  // If true, "rush install" will validate that the Git hook is correctly configured.
  // If false, then "rush prettier" commands will do nothing and print a warning that
  // Prettier is disabled.
  "stackPrettierEnabled": true,

  "prettierVersion": "2.0.5"

  // later this config file could also provide other features such as Prettier plugins
  // or autoconfiguration of the VS Code extension for Prettier
}

The Git hook will look like this:

common/config/git-hooks/pre-commit

# What the CLI parameters do:
# "--files=staged": process only the files staged with Git
# "--write": rewrite the files in place with the reformatted result, and restage them
# "--pre-commit": Use concise output appropriate for a Git hook
node common/scripts/install-run-rush.js prettier --files=staged --write --pre-commit || exit $?

"git commit" is a very fundamental user experience, so leveraging install-run-rush.js provides some advantages:

  • It works even if Rush is not installed yet in the current environment
  • The wrapper logic that we download and install is deterministically controlled by rushVersion from rush.json
  • Even if you are in a state where rush install would fail (e.g. your branch is broken), the Git hook will still function properly, because Prettier gets installed independently

I experimented with the pre-commit wrappers suggested by the Prettier website, but they all seem overengineered for our goals here. pretty-quick was the best one, but there are a few reasons I chose not to go with it:

  • The pretty-quick package has a peer dependency on prettier, which prevents it from being a Rush dependency; instead we would need to make a package.json for installing pretty-quick and pretty together
  • It brings in extra NPM dependencies; I'm trying to choose a design that will allow us to minimize the number of seconds for git commit in a repo where rush install was never run
  • It prints too much output on the console, and this isn't configurable

Invoking Prettier as a library looks pretty easy. It seems not much work to build this directly into Rush. (If it does turn out to be complicated then I would probably go with pretty-quick.)

The Rush CLI actions will be like this:

  • rush prettier --files=staged --write --pre-commit: Used by the pre-commit hook
  • rush prettier --files=all --write: Used initally to Prettify every file in the entire repo, according to .prettierignore. This is equivalent to prettier --write .
  • rush prettier --files=branch-diff --check or rush prettier --files=all --check: Can be invoked by CI for a PR build to validate that we didn't forget to run Prettier for some files

@octogonz
Copy link
Collaborator Author

octogonz commented Jun 8, 2020

Another possible approach:

We're considering modeling rush prettier as a custom command. In order for this to work, we would need two new features:

  • "shellCommandDefinesParameters": true Enables the target script to provide CLI help and validation, rather than defining it in custom-commands.json. This is similar to [rush] rushx should support parameters for commands #1232 for rushx, and can use the defineCommandLineRemainder() feature from PR [ts-command-line] Add defineCommandLineRemainder() feature #1869

  • "autoinstallFolder": "rush-prettier" Provide a way for a custom command to have its own package.json and pnpm-lock.yaml isolated from the rest of the monorepo. Rush will install this folder automatically when the command is invoked. These new "autoinstall" folders are meant for published tools or very small scripts; they should not rely on a toolchain, and thus are not modeled as Rush projects. Instead they go under common/autoinstall/task-name. In the future it might make sense to apply ensureConsistentVersions to them.

Here's an example of how it could work:

common/autoinstall/rush-prettier/package.json

{
  "name": "rush-prettier",
  "version": "1.0.0",
  "private": true,
  "dependencies": {
    "pretty-quick": "1.0.0",
    "prettier": "2.0.5"
  }
}

common/config/rush/common-versions.json

    {
      "name": "prettier",
      "commandKind": "global",
      "summary": "Invokes the Prettier command in a Rush repo",

      // The description is not used because "shellCommandDefinesParameters"=true
      // "description": "Invokes the Prettier command in a Rush repo",

      "autoinstallFolder": "rush-prettier",

      // This will invoke common/autoinstall/rush-prettier/node_modules/.bin/pretty-quick
      "shellCommand": "pretty-quick",

      "shellCommandDefinesParameters": true
    }

@octogonz
Copy link
Collaborator Author

octogonz commented Jun 9, 2020

This PR introduced the Prettier configuration #1922 and bulk-converted our repo.

@abdes
Copy link

abdes commented Aug 3, 2020

There is a serious headache I am facing and it brings up again one of the most annoying issues related to the combination of rush+vscode+tools:

  • the rush monorepo does not have a root package.json
  • many vscode extensions expect a package.json and a node_modules in the vscode workspace root :-(, as an example the esbenp.prettier-vscode and all its forks and derivatives...
  • the rush patterns for dealing with dependencies required by commands and repo tools is to either use auto-installers or make workspace package for tools, both of which install dependencies in their own nore-modules

@octogonz If you enhance a bit your prettierrc config to add, let's say a plugin, any prettier plugin, you will find out that vscode extensions don't work anymore, although your command line of course still works fine and prettier itself will work fine. This is a general issue related to the impact of not having a root package.json (which lerna has, Nx has, etc...). I'm not fond of that root package.json personally, but I am seriously annoyed by the amount of time I waste to get vscode working properly in a rush repo.

Do you have any feedback on this? or a proper way to get the rush auto-installer or repo tools package working with vscode?

@octogonz
Copy link
Collaborator Author

octogonz commented Nov 9, 2020

I'm closing this issue since we are now done with the Prettier and the approach is fully documented.

@abdes Sorry I overlooked your comment here. Some general answers:

  • It's perfectly fine to put package.json, as long as there are no dependencies being installed there.
  • For the large monorepo scenario that Rush targets, we recommend against opening VS Code in your root folder. It will be sluggish. Instead, we recommend to open separate VS Code windows for each project that you work on.
  • This VS Code extension aims to make it easy to switch between projects in this way.

If you have further questions/concerns, please open a dedicated GitHub issue. (This issue was about Prettier.)

@octogonz octogonz closed this as completed Nov 9, 2020
@prescience-data
Copy link

prescience-data commented Dec 20, 2020

There is a serious headache I am facing and it brings up again one of the most annoying issues related to the combination of rush+vscode+tools:
@octogonz If you enhance a bit your prettierrc config to add, let's say a plugin, any prettier plugin, you will find out that vscode extensions don't work anymore, although your command line of course still works fine and prettier itself will work fine. This is a general issue related to the impact of not having a root package.json (which lerna has, Nx has, etc...). I'm not fond of that root package.json personally, but I am seriously annoyed by the amount of time I waste to get vscode working properly in a rush repo.

Did you manage to get this working out of interest? I've got the same issue currently, trying to get "prettier-plugin-sorted" working.

@brycepolly
Copy link

There is a serious headache I am facing and it brings up again one of the most annoying issues related to the combination of rush+vscode+tools:
@octogonz If you enhance a bit your prettierrc config to add, let's say a plugin, any prettier plugin, you will find out that vscode extensions don't work anymore, although your command line of course still works fine and prettier itself will work fine. This is a general issue related to the impact of not having a root package.json (which lerna has, Nx has, etc...). I'm not fond of that root package.json personally, but I am seriously annoyed by the amount of time I waste to get vscode working properly in a rush repo.

Did you manage to get this working out of interest? I've got the same issue currently, trying to get "prettier-plugin-sorted" working.

I'm running into this also with import sorting, or any other prettier plugins. Did you get far with this @prescience-data or @abdes

@pswai
Copy link

pswai commented Jun 21, 2023

There is a serious headache I am facing and it brings up again one of the most annoying issues related to the combination of rush+vscode+tools:
@octogonz If you enhance a bit your prettierrc config to add, let's say a plugin, any prettier plugin, you will find out that vscode extensions don't work anymore, although your command line of course still works fine and prettier itself will work fine. This is a general issue related to the impact of not having a root package.json (which lerna has, Nx has, etc...). I'm not fond of that root package.json personally, but I am seriously annoyed by the amount of time I waste to get vscode working properly in a rush repo.

Did you manage to get this working out of interest? I've got the same issue currently, trying to get "prettier-plugin-sorted" working.

I'm running into this also with import sorting, or any other prettier plugins. Did you get far with this @prescience-data or @abdes

@brycepolly Any update from you?

I'm taking inspiration from #3230 (comment) and worked around it using the preRushInstall hook:

// rush.json
{
  // ...
  "eventHooks": {
    "preRushInstall": [
      "cp common/config/rush/.npmrc common/autoinstallers/rush-prettier",
      "common/temp/pnpm-local/node_modules/.bin/pnpm install --dir common/autoinstallers/rush-prettier"
    ],
  }
  // ...
}

That attempts to mimic what rush update-autoinstaller --name rush-prettier does, except that it won't be able to utilise rush hash to skip installation.

I tried being creative to execute rush update-autoinstaller --name rush-prettier here but it failed because there is already a rush process running.

@iclanton iclanton moved this to Closed in Bug Triage Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is asking for a new feature or design change
Projects
Archived in project
Development

No branches or pull requests

5 participants