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

Feature Request: nx format:check should provide a workaround for Prettier not supporting format diffing #4159

Closed
jacopolanzoni opened this issue Nov 26, 2020 · 61 comments
Assignees

Comments

@jacopolanzoni
Copy link

jacopolanzoni commented Nov 26, 2020

UPDATE FROM NX TEAM (2024-05-17)

Please read: #4159 (comment)


Description

As nx format:check shows which files fail the format check but not what is actually wrong in them, I would love if the --verbose option could actually add that information. At the moment, unless I am doing something wrong, nx format:check --verbose has the same output as nx format:check.

Motivation

The command would give more information about what is actually wrong in the file format.

Suggested Implementation

I would love if the --verbose option could actually add that information.

Alternate Implementations

As an alternative, I would like even only to see either the line where the check fails, or maybe the broken rule.

@github-actions
Copy link

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs.
If we missed this issue please reply to keep it active.
Thanks for being a part of the Nx community! 🙏

@konrad-garus
Copy link

It's like a slap in the face without ever even telling you why. Imagine a compiler telling you "Error, GFY, period".

Please add this.

@TCModus
Copy link

TCModus commented Nov 18, 2021

My entire team is complaining about this, going to have to remove my formatting check from pre-commit hook because it is just confusing everyone. The --verbose flag should be at minimum spitting out the same output of the format:write command which at least shows the command crawling through the project directories doing the formatting.

FYI for others with this issue it seems like you can run prettier using npm and get the output from running the prettier command directly instead of through Nx.

@Alspaladin
Copy link

Why it's closed? My project is failing on nx format:check without any message besides the error Command failed with exit code 1. and --verbose adds nothing to it

@eprokofev
Copy link

I don't understand "stale" logic as well. My pipeline also failed at this step without any reason. Verbose is not informative at all.

@xaphod
Copy link

xaphod commented Jan 12, 2022

Yeah, reopen this issue - verbose is broken

@KlausNie
Copy link

Same here

@konnic
Copy link

konnic commented Feb 22, 2022

Just ran into the same issue, would like to see this issue being reopened...

@konnic
Copy link

konnic commented Feb 22, 2022

If it helps anyone, running nx format:write solved issue for me. Nevertheless, it would be nice if the --verbose flag provided some extra info.

@TCModus
Copy link

TCModus commented Feb 22, 2022 via email

@StephenStrickland
Copy link

StephenStrickland commented May 4, 2022

nx format:check --verbose is still broken

edit:
It also appears that if you provide the base and head params the command exits with 1 but doesn't print the files that failed formatting.

e.g.

nx format:check --base=main --head=HEAD
# exit code 0 

nx format:check --base=main --head=HEAD --verbose
# exit code 0 

nx format:check
 >  NX   Affected criteria defaulted to --base=main --head=HEAD
/path/to/failed/file
# exit code 1

nx format:check --verbose
 >  NX   Affected criteria defaulted to --base=main --head=HEAD
/path/to/failed/file
# exit code 1

Unless there's some way to turn off the "warning" logging that nx has select a base and head, we can't even pipe the results of nx format:check and parse the results.

@shawnmclean
Copy link

This is passing locally and failing in CI for me. Both running on same node and npm versions.

@savvyshell
Copy link

How has this not been addressed , it's running fine locally, failing in CI with no information.

@savvyshell
Copy link

savvyshell commented Jun 22, 2022

init-commands: |
  npx nx-cloud record -- npx prettier --write .
  npx nx-cloud record -- npx nx format:write
  npx nx-cloud record -- npx prettier --check .
  npx nx-cloud record -- npx nx format:check

I added this to my actions.yml file and it seems to do the trick. Rather than rely on nx format:check, I hand it over to prettier since it's tied to my eslint configuration and I'd imagine nx format:check is using part of that anyways. I have it auto format with prettier followed by nx format:write just to be safe before checking with both prettier and nx format check.

It's probably not necessary to use both but just to still make sure NX is some-what happy, I wanted to use their format checker.

Seems to work now, hope we can get a proper solution for this in the future. At least with npx prettier --check, we get a bit more of a verbose output than format:check's tragic lack of insight in its output.

@MaxKless
Copy link
Collaborator

Please correct me if I'm wrong but it seems like all nx does is execute prettier under the hood anyways:

execSync(`node "${PRETTIER_PATH}" --list-different ${patterns.join(' ')}`, {

So if we want more verbose output, this will probably be the place to implement it.

@nhhockeyplayer
Copy link

nhhockeyplayer commented Jul 9, 2022

angular.json, nx.json, and tsconfig.base.json

That worked for me thanks friend

finally got nxcloud builds in flight

npmjs.org held me down for whole month in a user account debilitation locked out from their case logic creating users for emails that they dont even validate... ditched npmjs for github registry and I think this is the trend. sorry npmjs its been real.

nxcloud rocks... integrated to everything

do not release bad stuff into the public domain its going to get engineers very upset who are live in the field operating prod instances and thus far Nx and GitHub has been great. Wish I could say same for yarn, npm, webpack and other flanking entities. I know I wont sign my name to anything buggy. npmjs was last straw.

@pujux
Copy link

pujux commented Aug 1, 2022

why is this closed as completed if it's still a problem?

@Clean-Cole
Copy link

Our team is having the same issue.

@kekiel
Copy link

kekiel commented Aug 22, 2022

+1 for showing what is wrong with the file(s)

@dgesteves
Copy link

I am still having this issue with my CI any solution?

@StefanGreffenius
Copy link

StefanGreffenius commented Aug 25, 2022

Same issue here, we are missing the detailed error output of prettier. Would be great if you could reopen the issue.

@MaxKless MaxKless reopened this Sep 26, 2022
@github-actions github-actions bot removed the stale label Sep 27, 2022
@PayBas
Copy link

PayBas commented Sep 29, 2022

We parse our build logs to collect data on issues. The output of nx format:check is less than useless and is impossible to parse since there is nothing identifying the error, except the exit code.
I would at least expect some kind of line (even without the --verbose flag) like:

NX/Prettier format check error(s) for file(s):
<the file list>

But as others have mentioned, actually showing some verbose output is desperately needed.

@gerald-kalafut
Copy link

bump This is affecting my work flow as well. Please add descriptive text that identifies what is being flagged.

@amogower
Copy link

+1 for providing info on failed check please 🙏

@marc-wilson
Copy link

I am confused as well. I am evaluating Nx and am playing around with their Azure DevOps example here.

The documentation offers this snippet:

trigger:
  - main
pr:
  - main

variables:
  CI: 'true'
  ${{ if eq(variables['Build.Reason'], 'PullRequest') }}:
    NX_BRANCH: $(System.PullRequest.PullRequestNumber)
    TARGET_BRANCH: $[replace(variables['System.PullRequest.TargetBranch'],'refs/heads/','origin/')]
    BASE_SHA: $(git merge-base $(TARGET_BRANCH) HEAD)
  ${{ if ne(variables['Build.Reason'], 'PullRequest') }}:
    NX_BRANCH: $(Build.SourceBranchName)
    BASE_SHA: $(git rev-parse HEAD~1)
  HEAD_SHA: $(git rev-parse HEAD)

jobs:
  - job: main
    pool:
      vmImage: 'ubuntu-latest'
    steps:
      - script: npm ci

      - script: npx nx format:check

      - script: npx nx affected --base=$(BASE_SHA) -t lint --parallel=3
      - script: npx nx affected --base=$(BASE_SHA) -t test --parallel=3 --configuration=ci
      - script: npx nx affected --base=$(BASE_SHA) -t build --parallel=3

But, I get an error when it runs npx nx format:check

fatal: ambiguous argument 'master': unknown revision or path not in the working tree.

Using a suggestion in another comment (changing it from check to write), I get the same error in the logs, but it doesn't return an exit code of 1 and allows my pipeline to continue on to the next build task.

Full build logs

with npx nx format:check

Starting: CmdLine
==============================================================================
Task         : Command line
Description  : Run a command line script using Bash on Linux and macOS and cmd.exe on Windows
Version      : 2.212.0
Author       : Microsoft Corporation
Help         : https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/command-line
==============================================================================
Generating script.
Script contents:
npx nx format:check
========================== Starting Command Output ===========================
/usr/bin/bash --noprofile --norc /home/vsts/work/_temp/38edf7c3-b384-4c31-bdc1-dee7fbdc928f.sh
fatal: ambiguous argument 'master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
apps/api-e2e/jest.config.ts
apps/api-e2e/src/api/api.spec.ts
apps/api-e2e/src/support/global-setup.ts
apps/api-e2e/src/support/test-setup.ts
apps/api/jest.config.ts
apps/api/src/app/app.controller.spec.ts
apps/api/src/app/app.controller.ts
apps/api/src/app/app.module.ts
apps/api/src/app/app.service.spec.ts
apps/api/src/app/app.service.ts
apps/api/src/main.ts
apps/api/webpack.config.js
apps/app/src/app/app.component.ts
jest.config.ts
jest.preset.js
##[error]Bash exited with code '1'.
Finishing: CmdLine

with npx nx format:write

Starting: CmdLine
==============================================================================
Task         : Command line
Description  : Run a command line script using Bash on Linux and macOS and cmd.exe on Windows
Version      : 2.212.0
Author       : Microsoft Corporation
Help         : https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/command-line
==============================================================================
Generating script.
Script contents:
npx nx format:write
========================== Starting Command Output ===========================
/usr/bin/bash --noprofile --norc /home/vsts/work/_temp/27daa746-95fc-41d1-9993-5b0509e5e408.sh
fatal: ambiguous argument 'master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
apps/api-e2e/jest.config.ts
apps/api-e2e/src/api/api.spec.ts
apps/api-e2e/src/support/global-setup.ts
apps/api-e2e/src/support/test-setup.ts
apps/api/jest.config.ts
apps/api/src/app/app.controller.spec.ts
apps/api/src/app/app.controller.ts
apps/api/src/app/app.module.ts
apps/api/src/app/app.service.spec.ts
apps/api/src/app/app.service.ts
apps/api/src/main.ts
apps/api/webpack.config.js
apps/app/src/app/app.component.ts
jest.config.ts
jest.preset.js
tsconfig.base.json
Finishing: CmdLine

I created a documentation issue (#16241) for this initially, but it sounds like this command may be having troubles in a pipeline?

@ak274
Copy link

ak274 commented Apr 13, 2023

I'm also facing this issue in gitlab ci npx nx format:check fails without telling what's wrong! Wasted many hours and then come here to look into this issue

@tomavic
Copy link

tomavic commented Apr 16, 2023

I had same issue too and it's really confusing. Is it main or master :/

My ci.yml

name: Publish Libraries
'on':
  push:
    branches:
      - main
  pull_request:

jobs:
  build_and_deploy:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout repo ✅
        uses: actions/checkout@v2
      - name: Install Dependencies 🔧
        run: |
          if [ -e yarn.lock ]; then
          yarn install --frozen-lockfile
          elif [ -e package-lock.json ]; then
          npm ci
          else
          npm i
          fi
      - name: Pretty code 💄
        run: npx nx format:check
      - name: Lint ♻️
        run: npx nx affected -t lint
      - name: Build 📦
        run: npx nx affected -t build
      - name: Test 🧪
        run: npx nx affected -t test --ci --code-coverage
      - name: Release 🚀
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        run: npx nx affected -t release

and it fails at the linting stage, saying

Run npx nx affected -t lint
  npx nx affected -t lint
  shell: /usr/bin/bash -e {0}

 >  NX   Affected criteria defaulted to --base=master --head=HEAD

fatal: ambiguous argument 'master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
nx affected

Run target for affected projects

Run command using --base=[SHA1] (affected by the committed, uncommitted and untracked changes):
      --base  Base of the current branch (usually main)  [string]

or using --base=[SHA1] --head=[SHA[2](https://github.com/tomavic/enigma-nx-angular/actions/runs/4710392822/jobs/8353993527?pr=2#step:5:2)] (affected by the committed changes):
      --base  Base of the current branch (usually main)  [string]
      --head  Latest commit of the current branch (usually HEAD)  [string]

@pujux
Copy link

pujux commented Apr 19, 2023

@tomavic you are encountering a different issue. you need to add fetch-depth to your actions/checkout action to have all history you need for nx.

from https://github.com/actions/checkout#readme

# Number of commits to fetch. 0 indicates all history for all branches and tags.
# Default: 1
fetch-depth: ''

also have a look at: https://github.com/nrwl/nx-set-shas

@tomavic
Copy link

tomavic commented Apr 20, 2023

@tomavic you are encountering a different issue. you need to add fetch-depth to your actions/checkout action to have all history you need for nx.

from https://github.com/actions/checkout#readme

# Number of commits to fetch. 0 indicates all history for all branches and tags.
# Default: 1
fetch-depth: ''

also have a look at: https://github.com/nrwl/nx-set-shas

Thanks @pujux That is a really different issue and I've fixed it using the correct usage of actions/checkout@v2 and https://github.com/nrwl/nx-set-shas ✔️

@zhumeisongsong
Copy link

Same here, failing in ci but working locally.

@chief-austinc
Copy link

+1

@chaunceyau
Copy link

opened an MR to address this here: #4159, open to feedback

@Oliviu27
Copy link

Same here, the fact that it's been almost 2.5 years without a fix to such a obvious issue is sad

@abhi40308
Copy link

abhi40308 commented Jun 22, 2023

If your format check is failing on CI but working locally, you might have some files which are being generated in CI, which might not be formatted properly. You can add those files to .prettierignore. In my case, npm ci was generating mockServiceWorker.js, which was failing on lint check. This got resolved after adding it to .prettierignore.

Workflow:

name: lint frontend

on: [push]
jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
        with:
          ref: ${{ github.event.pull_request.head.ref }}
          fetch-depth: 0
      - uses: actions/setup-node@v3
        with:
          node-version: '16'
      - run: yarn install --immutable
      # Note: nx format:check does the `nx affected` check automatically, so it needs a ref to compare against
      - run: git fetch --no-tags --prune --depth=5 origin main
      - run: yarn nx format:check --base=main --head=HEAD
      - run: yarn eslint:check

.prettierignore

# Add files here to ignore them from prettier formatting

/dist
/coverage
/.vscode
/node_modules
/tools/msw/mockServiceWorker.js

If your format check is failing on CI but working locally

You can verify this as not being a problem with nx by replacing nx format:check and just run prettier directly on CI yarn prettier -c . should still fail with the same output.

@lukket
Copy link

lukket commented Jul 26, 2023

I really wonder how this command made it into production. The documentation actually says nothing about what the command really does, instead it's written somewhere else. And the output is just a bad joke 🙈.

@tamoore
Copy link

tamoore commented Aug 7, 2023

Seriously, why does it output nothing but the affected files. Yes you can write to fix, but it's so unbelievably frustrating to have no idea what the output is.

@algoflows
Copy link

I've blasted up there youtube threads with links too this issue, maybe the team will finally address it.

@davidplappert
Copy link

please fix ...

@Israeltheminer
Copy link

Why hasn't this issue been formally addressed yet? this should be fixed

@jasongerbes
Copy link
Contributor

@vsavkin, this issue was raised nearly 3 years ago. It has the most 👍 and ❤️ reactions of any issue for this project, and has about 46 subscribers.

nx format:check is essentially unusable in it's current state, yet it's still included in the Nx docs.

Can you please give an update on whether this issue will ever be addressed, or mark nx format:check and nx format:write as deprecated and provide an alternative (e.g. npx prettier --check . and npx prettier --write .).

@riencoertjens
Copy link
Contributor

I added a message on their discord if everyone here replies to that maybe they'll notice something? 🤷🏻‍♂️

@pujux
Copy link

pujux commented Sep 25, 2023

So I mentioned Miroslav Jonas on Twitter to try and see if the team has taken notice on the issue and here is his reply: https://x.com/meeroslav/status/1706345582242001064?s=46&t=ZnrURlqYmjZQZciRM14ZDA

we might be getting a solution soon 🥳

@jared-christensen
Copy link

Hey everyone,

We've found it challenging to determine when nx format:check fails because it merely lists out files without any additional context about the formatting issues. To make this clearer for our team, I created a bash script. This script not only runs nx format:check, but it also explicitly highlights the files with formatting errors and recommends running npx nx format:write to address these issues.

Here is an example of it in action:
1562806300-Window_and_package_json_—_dice-web-platform_—_Dice_Web_Platform

Here's the script:

pretty-format-check.sh

#!/bin/bash

# ANSI color codes
RED='\033[0;31m'
GREEN='\033[0;32m'
CYAN='\033[0;36m'
NC='\033[0m'
SEPARATOR="----------------------------------------"

# Run nx pretty-format-check and capture the output
output=$(nx format:check 2>&1)

# Capture the exit status
result=$?

# Check if the command failed
if [ $result -ne 0 ]; then
    echo -e "${RED}$SEPARATOR"
    echo -e "Error: Formatting check failed"
    echo -e "$SEPARATOR${NC}"
    echo -e "Files with formatting issues:\n$output\n"
    echo -e "${CYAN}Please run 'npx nx format:write' to fix formatting issues."
    exit $result
fi

echo -e "${GREEN}Formatting check passed successfully.${NC}"

To use this script, save it as pretty-format-check.sh and make sure it's executable. You can do this with:

chmod +x pretty-format-check.sh

Also, I've added it as an npm script in package.json for convenience:

"scripts": {
    "pretty-format-check": "./pretty-format-check.sh"
}

Now, you can simply run npm run pretty-format-check to execute the script.

Hope this helps make formatting checks a breeze!

@JamesHenry
Copy link
Collaborator

Hi Folks,

Nx format is currently exclusively built on top of prettier. For nx format:check, we leverage the --list-different feature of prettier to show the user which files failed the formatting check just like prettier itself does https://prettier.io/docs/en/cli.html#--list-different).

The simple common case solution is to run nx format:write (powered by prettier --write) and commit the result.

To summarize this thread, however:

Right now a user might want to try and learn more about their format:check failure by passing --verbose but it will not change the output in any way.

Based on the feedback in this issue some users are expecting to see the diff output of what correct vs incorrect formatting would look like. However, because nx format is currently exclusively built on top of prettier, it is therefore subject to its limitations. prettier/prettier#6885 remains unresolved and so there is no native way for users to see a diff.

It is important to note that only showing the names of the files in the default nx format:check output is an important feature - it allows easy piping into custom scripts to perform further handling. Therefore we do not want to change the default output here.

I have opened PR #23503 which updates the case where --verbose is passed in order to provide more information/context about the overall problem and how to fix it, including linking to the prettier limitation which is at the heart of this.

Before

image

After

image


Notes on actually diffing

Naturally, what we recommend is to continue to give feedback to the Prettier team that you want this feature.

However, if a user here would like to take things a step further and prototype what it might look like for Nx to attempt to make up for the lack of diffing in prettier, then please do, we would be willing to consider a PR for this if a reliable solution can be found.

However, it is worth noting that this behavior should not be activated by simply passing --verbose because

  1. It will be expensive (and the number of failing files could be very large in certain situations)
  2. It will have to be powered by side-effectful disk and git operations, which is not what --verbose is intended to perform

Instead, it would have to be powered by some opt-in additional flag, such as --diff.

@JamesHenry JamesHenry changed the title nx format:check --verbose should actually show what is wrong with a file Feature Request: nx format:check should provide a workaround for Prettier not supporting format diffing May 17, 2024
@JamesHenry
Copy link
Collaborator

I have updated the issue description to more accurately reflect the nature of this request and provided a link to my most recent comment as the latest status on this.

Thanks all!

Copy link

This issue has been automatically marked as stale because it hasn't had any activity for 6 months.
Many things may have changed within this time. The issue may have already been fixed or it may not be relevant anymore.
If at this point, this is still an issue, please respond with updated information.
It will be closed in 21 days if no further activity occurs.
Thanks for being a part of the Nx community! 🙏

@github-actions github-actions bot added the stale label Nov 14, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests