Update `package.json` to do `npx tsc` in `postinstall` to fix `npx prettier` #23667

raych1 · 2023-04-21T02:36:31Z

@weshaggard , some users reported the issue (e.g. azure/azure-sdk-tools #6017)

npm run prettier -- --write specification/**/*.json

> prettier
> prettier --write specification/**/*.json

[error] Cannot find module '/home/raychen/tmp/azure-rest-api-specs/scripts/prettier-swagger-plugin'
[error] Require stack:
[error] - /home/raychen/tmp/azure-rest-api-specs/node_modules/prettier/index.js
[error] - /home/raychen/tmp/azure-rest-api-specs/node_modules/prettier/cli.js
[error] - /home/raychen/tmp/azure-rest-api-specs/node_modules/prettier/bin-prettier.js

It needs to run tsc after run npm install. So I added back the build step of post npm install.

~~The aka.ms/ci-fix also needs to be updated to remove the npm run prettier part as prettier script was removed from package.json.~~

Fixed by kojamroz by additional commits pushed to this PR.

Added back the build step post npm install to fix `npm run prettier-fix` error. otherwise, user needs to run `tsc` manually before run the prettier-fix.

openapi-workflow-bot · 2023-04-21T02:36:34Z

Hi, @raych1 Thanks for your PR. I am workflow bot for review process. Here are some small tips.

Please ensure to do self-check against checklists in first PR comment.

PR assignee is the person auto-assigned and responsible for your current PR reviewing and merging.

For specs comparison cross API versions, Use API Specs Comparison Report Generator

If there is CI failure(s), to fix CI error(s) is mandatory for PR merging; or you need to provide justification in PR comment for explanation. How to fix?

Any feedback about review process or workflow bot, pls contact swagger and tools team. [email protected]

openapi-pipeline-app · 2023-04-21T02:36:35Z

Swagger Validation Report

️️✔️BreakingChange succeeded [Detail] [Expand]

There are no breaking changes.

️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand]

There are no breaking changes.

️️✔️CredScan succeeded [Detail] [Expand]

There is no credential detected.

️️✔️LintDiff succeeded [Detail] [Expand]

Validation passes for LintDiff.

️️✔️Avocado succeeded [Detail] [Expand]

Validation passes for Avocado.

️️✔️ApiReadinessCheck succeeded [Detail] [Expand]

️️✔️~[Staging] ServiceAPIReadinessTest succeeded [Detail] [Expand]

Validation passes for ServiceAPIReadinessTest.

️️✔️SwaggerAPIView succeeded [Detail] [Expand]

️️✔️CadlAPIView succeeded [Detail] [Expand]

️️✔️TypeSpecAPIView succeeded [Detail] [Expand]

️️✔️ModelValidation succeeded [Detail] [Expand]

Validation passes for ModelValidation.

️️✔️SemanticValidation succeeded [Detail] [Expand]

Validation passes for SemanticValidation.

️️✔️PoliCheck succeeded [Detail] [Expand]

Validation passed for PoliCheck.

️️✔️PrettierCheck succeeded [Detail] [Expand]

Validation passes for PrettierCheck.

️️✔️SpellCheck succeeded [Detail] [Expand]

Validation passes for SpellCheck.

️️✔️Lint(RPaaS) succeeded [Detail] [Expand]

Validation passes for Lint(RPaaS).

️️✔️CadlValidation succeeded [Detail] [Expand]

Validation passes for CadlValidation.

️️✔️TypeSpec Validation succeeded [Detail] [Expand]

Validation passes for TypeSpec Validation.

️️✔️PR Summary succeeded [Detail] [Expand]

Validation passes for Summary.

_{Posted by Swagger Pipeline | How to fix these errors?}

openapi-pipeline-app · 2023-04-21T02:36:36Z

Swagger pipeline restarted successfully, please wait for status update in this comment.

openapi-pipeline-app · 2023-04-21T02:36:36Z

Swagger pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

weshaggard · 2023-04-21T16:04:32Z

package.json

@@ -22,6 +22,7 @@
  "scripts": {
    "prettier-check": "ts-node ./scripts/prettier-check.ts",
    "prettier-fix": "prettier --write specification/**/*.json"
+    "postinstall": "tsc"


Why is this needed?

I think the idea is this will run npx tsc which is a prerequisite for running npx prettier-fix which is prettier --write.

https://docs.npmjs.com/cli/v9/using-npm/scripts

I understand tsc compiles any source it finds in the repo but that seems like it is a little overkill just to get prettier working. What I'm trying to understand is do we have other options? Also I'd like to better understand the difference between this prettier script vs what we do in the unified pipeline which is slightly different I believe. We should see if we can make these exactly the same.

I agree that running tsc in postinstall seems like overkill just to get prettier working. I will try to understand what we are trying to do here, why it's broken if you don't run tsc, and the best way to fix it.

An alternative would be to run tsc as a pre-step before the prettier scripts:

"scripts": { "preprettier-check": "tsc", "prettier-check": "ts-node ./scripts/prettier-check.ts",

However, running tsc from scripts fails for me, regardless of whether it's run in preprettier-check or postinstall:

~/azure-rest-api-specs$ npm install > postinstall > tsc -p /home/mharder/azure-rest-api-specs node_modules/@types/node/ts4.8/test.d.ts:612:34 - error TS1005: '?' expected. 612 : F extends abstract new (...args: any) => infer T ~~~

But running tsc directly from the command line works with no errors:

~/azure-rest-api-specs$ tsc --build --verbose [9:49:06 PM] Projects in this build: * tsconfig.json [9:49:06 PM] Project 'tsconfig.json' is up to date because newest input 'scripts/prettier-check.ts' is older than output 'scripts/utils.js'

The problem with tsc above, is the version of typescript in package.json and package-lock.json is too old to compile the types installed by package @types/node. It works when I run tsc directly from the command line, because I have a newer version of typescript installed globally. However, running tsc from the scripts section uses the version of typescript installed locally, which is too old (3.9.10). You can repro this from the command line by running npx tsc instead of tsc.

So I think I would recommend the following:

Update the version of typescript in package.json and package-lock.json to the latest compatible version.

Add steps preprettier-check and preprettier-fix that call tsc. This makes these scripts a little slower since they need to run tsc every time, but I like this better than running tsc in a global postinstall when it's only needed for prettier.

@weshaggard , the difference in unified pipeline is the scripts under \scripts folder is compiled JS but in spec repo they are *.ts file. That's why tsc command needs to run to turn them into JS. I don't worry about the perf as only \scripts folder might contain *.ts files. specification folder shouldn't have *.ts files.

@mikeharder , whatever the typescript version in package.json needs to be fixed. I would prefer to wrap tsc in postinstall command as it would be needed for other scenarios rather than prettier only. @konrad-jamrozik , can you help with typescript version upgrade?

I am fine with adding tsc to postinstall, I agree it doesn't add much time to the npm install command.

openapi-pipeline-app · 2023-04-21T21:29:37Z

Swagger Generation Artifacts

️️✔️ApiDocPreview succeeded [Detail] [Expand]

️️✔️SDK Breaking Change Tracking succeeded [Detail] [Expand]

Breaking Changes Tracking

_{Posted by Swagger Pipeline | How to fix these errors?}

konrad-jamrozik · 2023-04-21T21:44:19Z

documentation/ci-fix.md

@@ -21,11 +21,21 @@ Or if you want to fix specified service, use the complete path, not relative:

 ```
 npm install
-npm run prettier -- --write "<path to repo>/azure-rest-api-specs/specification/**/*.json"
+npx prettier --write "<path to repo clone>/azure-rest-api-specs/specification/**/*.json"


FYI the need for this to be npx is explained here: https://prettier.io/docs/en/cli.html.

At the same time, one can call npm run prettier-fix instead of something like npx prettier-fix because npm run executes this from package.json: "prettier-fix": "prettier --write specification/**/*.json", i.e. the prettier executable, which is also explained here: https://prettier.io/docs/en/cli.html.

@mikeharder any thoughts on the best way to run prettier? Should we just use npx all the time and avoid having the scripts in the project.json? Also if you use npx does it read the version from package.json or does it always run the latest version? I'm still trying to sort out the usages best options when to use npx or not.

One aspect is to install prettier (and its transitive deps) from a lockfile, to guarantee repeatable builds. I think this should already be covered by the package-lock.json file in the repo root.

I'm pretty sure npx will use the version of prettier (and transitive deps) installed from the lockfile, but it would be good to test this to verify.

If you run npx prettier in the repo before running npm install, it tries to install latest, so it appears this is ignoring the package-lock.json in the root:

~/azure-rest-api-specs$ git clean -xdf Removing node_modules/ ~/azure-rest-api-specs$ npx prettier --version Need to install the following packages: [email protected] Ok to proceed? (y) n

However, if you run npm install first, then npx prettier uses the version installed from the lockfile:

~/azure-rest-api-specs$ npm install added 247 packages, and audited 248 packages in 4s ~/azure-rest-api-specs$ npm ls | grep prettier ├── @types/[email protected] ├── [email protected] ~/azure-rest-api-specs$ npx prettier --version 2.8.4

I don't see any way to tell npx to install from the lockfile if the requested package is not already installed. We could tell people to use npx --no-install, which changes behavior to fail with an error instead of installing latest, but unfortunately the error message doesn't tell you why it failed (and neither does the logfile):

~/azure-rest-api-specs$ npx --no-install prettier --version npm ERR! canceled npm ERR! A complete log of this run can be found in: npm ERR! /home/mharder/.npm/_logs/2023-04-24T20_11_46_807Z-debug-0.log

I think our best option will be to tell users to run a script like npm run prettier-fix, this way we can modify the prettier-fix script in package.json to do whatever we want, including running tsc (ideally on only the needed directories) before running prettier. If we tell users to directly call npx prettier, we have no way to modify the behavior or run pre/post steps.

Thanks @mikeharder this is exactly the type of advise I was looking for. I agree we should avoid telling folks to use npx as it isn't necessarily consistent with what we have in our package.json and package.lock.json file. I think npm install and npm run prettier-fix is the best option.

One challenge with npm scripts is if you need to pass different arguments. For example, the current prettier-fix script calls prettier --write specification/**/*.json. So if you wanted to only run this command on a subdirectory, you'd need to either use npx prettier from the command-line, or we could add another script named just prettier so you can use npm run prettier -- <your-arguments-here>:

"scripts": { "preprettier": "tsc", "prettier": "prettier",

I think they will want to scope the run so having the prettier script should be fine. We could also update the prettier-fix to not pass the full path but allow that to be passed as a parameter and then have a perttier-fix-all or something like that to cover all if needed.

raych1 · 2023-04-24T00:30:42Z

I added one more library reference.

This is the failed PR FYI.

Added tcgc library reference.

Fixed missing quotation mark.

package.json

weshaggard · 2023-04-27T23:41:12Z

Closing this in favor of #23763

Update package.json

fa0fde3

Added back the build step post npm install to fix `npm run prettier-fix` error. otherwise, user needs to run `tsc` manually before run the prettier-fix.

raych1 requested a review from weshaggard April 21, 2023 02:36

raych1 assigned weshaggard Apr 21, 2023

Update ci-fix.md

0ed1893

weshaggard reviewed Apr 21, 2023

View reviewed changes

weshaggard requested a review from konrad-jamrozik April 21, 2023 16:05

Update ci-fix.md: fix bug + add troubleshooting

dd38d5c

konrad-jamrozik approved these changes Apr 21, 2023

View reviewed changes

konrad-jamrozik self-requested a review April 21, 2023 21:29

Update ci-fix.md: fixes to prettier doc

6a9109f

konrad-jamrozik reviewed Apr 21, 2023

View reviewed changes

Update ci-fix.md: fix typo

cdea381

konrad-jamrozik changed the title ~~Update package.json to add back postinstall script~~ Update package.json to do npx tsc in postinstall to fix npx prettier Apr 21, 2023

raych1 added 2 commits April 24, 2023 21:16

Update package.json

69a967b

Added tcgc library reference.

Update package.json

06928ec

Fixed missing quotation mark.

weshaggard reviewed Apr 24, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

Revert the change of adding tcgc library reference

dfbaa31

konrad-jamrozik mentioned this pull request Apr 25, 2023

Improve PrettierCheck failures to make them more actionable Azure/azure-sdk-tools#5992

Open

weshaggard closed this Apr 27, 2023

konrad-jamrozik deleted the user/raych1/fix-prettier-run-error branch December 8, 2023 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `package.json` to do `npx tsc` in `postinstall` to fix `npx prettier` #23667

Update `package.json` to do `npx tsc` in `postinstall` to fix `npx prettier` #23667

raych1 commented Apr 21, 2023 •

edited by konrad-jamrozik

Loading

openapi-workflow-bot bot commented Apr 21, 2023

openapi-pipeline-app bot commented Apr 21, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 21, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 21, 2023 •

edited

Loading

weshaggard Apr 21, 2023

konrad-jamrozik Apr 21, 2023 •

edited

Loading

weshaggard Apr 24, 2023

mikeharder Apr 24, 2023

mikeharder Apr 24, 2023 •

edited

Loading

mikeharder Apr 24, 2023

raych1 Apr 25, 2023

mikeharder Apr 25, 2023

openapi-pipeline-app bot commented Apr 21, 2023 •

edited

Loading

Breaking Changes Tracking

konrad-jamrozik Apr 21, 2023 •

edited

Loading

weshaggard Apr 24, 2023

mikeharder Apr 24, 2023

mikeharder Apr 24, 2023

mikeharder Apr 24, 2023

mikeharder Apr 24, 2023

weshaggard Apr 24, 2023

mikeharder Apr 24, 2023

weshaggard Apr 24, 2023

raych1 commented Apr 24, 2023 •

edited

Loading

weshaggard commented Apr 27, 2023

Update package.json to do npx tsc in postinstall to fix npx prettier #23667

Update package.json to do npx tsc in postinstall to fix npx prettier #23667

Conversation

raych1 commented Apr 21, 2023 • edited by konrad-jamrozik Loading

openapi-workflow-bot bot commented Apr 21, 2023

openapi-pipeline-app bot commented Apr 21, 2023 • edited Loading

Swagger Validation Report

openapi-pipeline-app bot commented Apr 21, 2023 • edited Loading

openapi-pipeline-app bot commented Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

konrad-jamrozik Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharder Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openapi-pipeline-app bot commented Apr 21, 2023 • edited Loading

Swagger Generation Artifacts

Breaking Changes Tracking

konrad-jamrozik Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raych1 commented Apr 24, 2023 • edited Loading

weshaggard commented Apr 27, 2023

Update `package.json` to do `npx tsc` in `postinstall` to fix `npx prettier` #23667

Update `package.json` to do `npx tsc` in `postinstall` to fix `npx prettier` #23667

raych1 commented Apr 21, 2023 •

edited by konrad-jamrozik

Loading

openapi-pipeline-app bot commented Apr 21, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 21, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 21, 2023 •

edited

Loading

konrad-jamrozik Apr 21, 2023 •

edited

Loading

mikeharder Apr 24, 2023 •

edited

Loading

openapi-pipeline-app bot commented Apr 21, 2023 •

edited

Loading

konrad-jamrozik Apr 21, 2023 •

edited

Loading

raych1 commented Apr 24, 2023 •

edited

Loading