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

Doctor should treat install errors like test errors #1170

Closed
3 tasks done
kayahr opened this issue Jul 26, 2022 · 6 comments
Closed
3 tasks done

Doctor should treat install errors like test errors #1170

kayahr opened this issue Jul 26, 2022 · 6 comments
Labels

Comments

@kayahr
Copy link

kayahr commented Jul 26, 2022

  • I have searched for similar issues
  • I am using the latest version of npm-check-updates
  • I am using node >= 14

The doctor iteration doesn't work for any project which does some kind of compilation/transpilation (Typescript, native stuff, ...) in the prepare stage because in this case after upgrading a dependency the npm install step will most likely fail because this triggers the compilation and TypeScript for example throws a compilation error when the compiled code is not compatible with the new dependency. But when npm install fails then the doctor immediately aborts.

It would be nice if the doctor could treat npm install errors the same as when errors occurred during npm test instead of aborting. Otherwise TypeScript projects and other similar projects which performs compilation in the prepare stage can't be doctored by ncu.

@raineorshine
Copy link
Owner

Thanks for the thorough explanation. I totally agree.

I have attempted a fix and published it to v16.0.1. Try it and let me know if it works as expected.

@kayahr
Copy link
Author

kayahr commented Jul 26, 2022

Hm... I'm afraid it didn't work as expected. The following happens :

$ ncu --doctor -u
Running tests before upgrading
npm install                       
npm run test
Upgrading all dependencies and re-running tests
ncu -u
npm install
Tests failed
Identifying broken dependencies
npm install --no-save tslib@^2.4.0
npm run test
  ✓ tslib ^2.3.1 → ^2.4.0
npm install --no-save [email protected]
npm run test
  ✓ jest-extended 2.0.0 → 3.0.1
Saving partially upgraded package.json
npm install

/home/k/.local/lib/node_modules/npm-check-updates/build/src/index.js:56
    throw err;
    ^
npm ERR! code 2
npm ERR! path /home/k/Projects/topaz/topaz
npm ERR! command failed
npm ERR! command sh /tmp/prepare-dfcc16cc.sh
{
  "error": {
    "code": 2,
    "summary": "command failed",
    "detail": "sh /tmp/prepare-dfcc16cc.sh"
  }
}

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/k/.npm/_logs/2022-07-26T14_00_46_383Z-debug-0.log

(Use `node --trace-uncaught ...` to show where the exception was thrown)

So when running npm install after applying ALL updates the Doctor correctly sees that the project is no longer compiling (Because jest-extended messed up the typings in v3.0.1...). But during the iteration over the single updates no npm install is called again. Instead for example npm install --no-save [email protected] is called which just installs the dependency but doesn't compile the project again because the prepare script is not called. So the incremental update "looks" ok but it isn't. At the end ncu calls npm install again which triggers the compilation which now fails and Doctor aborts.

Hm, I'm not sure how this can be fixed. I assumed ncu doctor would apply a single update to the package.json and then runs npm install. Then it would work. Or alternatively ncu could run npm run prepare after installing the single dependency. But this must only be done if there actually is a prepare script in the package.json or otherwise this fails.

I know this is a special case for mostly TypeScript/CoffeScript projects. But even the NPM documentation says that the prepare script (or the deprecated prepublish) can be used for "Compiling CoffeeScript source code into JavaScript." so I think it is safe to say that this is the common standard and it would be nice if ncu could support this.

@raineorshine
Copy link
Owner

raineorshine commented Jul 26, 2022

Ah, I see. I didn't know that npm prepare was not executed by npm install --no-save.

Do you have a repo where I can reproduce the issue?

I think I will add npm prepare after each individual install. To clarify, is the prepare script that needs to run in your project, or in the dependency?

@kayahr
Copy link
Author

kayahr commented Jul 31, 2022

Here is an example for reproduction:

https://github.com/kayahr/ncu-1170

The prepare script is in my project, not in the dependency.

@raineorshine
Copy link
Owner

raineorshine commented Jul 31, 2022

Thanks so much! That's super helpful. I was able to turn it into an automated test.

I added the logic to manually run the prepare script when it is present.

Published to v16.0.4

@kayahr
Copy link
Author

kayahr commented Jul 31, 2022

Works great now. Thank you for the fast response and fix and thanks for this wonderful tool!

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

No branches or pull requests

2 participants