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

Deprecation messages #206

Closed
gingermusketeer opened this issue Aug 5, 2021 · 14 comments
Closed

Deprecation messages #206

gingermusketeer opened this issue Aug 5, 2021 · 14 comments

Comments

@gingermusketeer
Copy link

I am using a tool called oclif to build a CLI and this tool inspects errors that are thrown to see how they should be shown to the user. Unfortunately this is triggering some deprecation warnings from request-error.

The code that is causing the deprecation is const shouldPrint = !(err.code === 'EEXIT'); and the logged message is

Deprecation: [@octokit/request-error] `error.code` is deprecated, use `error.status`.

Is there anyway to avoid seeing these deprecation messages?

@gr2m
Copy link
Contributor

gr2m commented Aug 5, 2021

Hmm the error will go away once we release a breaking version. But if you like we could check if error.code is a number between 100 and 599, and only log the deprecation message in that case? I'd accept a pull request if you are up to it

@gingermusketeer
Copy link
Author

@gr2m How long do you think this deprecation error will be in place for?

I don't think adding a check would help as the error code in my case is 401. The oclif framework is calling the .error method to check if it is some other kind of error. I think having an option to disable the deprecation or opt in to the new behaviour would be the only way forward. What do you think?

@gr2m
Copy link
Contributor

gr2m commented Aug 6, 2021

How long do you think this deprecation error will be in place for?

I don't know. We will definitely have a breaking change coming this year

I think having an option to disable the deprecation or opt in to the new behaviour would be the only way forward

That would require a lot of changes in @octokit/* packages in order to pass through that option, it's too much of an edge case for that, sorry

@gingermusketeer
Copy link
Author

@gr2m What about an API within this package that I can call directly? Thinking something like:

import {disableDeprecationMessages} from "@oktokit/error"
disableDeprecationMessages()

@gr2m
Copy link
Contributor

gr2m commented Aug 9, 2021

no sorry, I won't add any code to @error/code for this particular case, it's just not common enough.

When you do your check

const shouldPrint = !(err.code === 'EEXIT');

maybe add a check above such as

const isOctokitRequestError = err.name === 'HttpError' && "status" in err
const shouldPrint = !isOctokitRequestError && !(err.code === 'EEXIT');

@gingermusketeer
Copy link
Author

The code you suggested won't work for me as the it is inside a module that I depend on and not code I maintain. Thanks for the feedback however.

@gingermusketeer
Copy link
Author

Found a way around this:

/* Octokit monkey patch to silence un-fixable deprecation warnings */
import { Deprecation } from "deprecation";
const oldWarn = console.warn;
console.warn = (...args: any[]) => {
  const isDeprecation = args.some((arg) => arg instanceof Deprecation);
  if (isDeprecation) {
    // Silence is golden.
    return;
  }
  oldWarn.apply(console, args);
};
/* End of monkey patch */

@till
Copy link
Contributor

till commented May 25, 2023

@gr2m I npm installed'd yesterday, and keep getting these errors. This is among the first Google results. Any update as to how to get rid off these errors?

@gr2m
Copy link
Contributor

gr2m commented May 26, 2023

Right above you: #206 (comment)

@till
Copy link
Contributor

till commented May 26, 2023

That's still the only way? Guess I was wondering why the lib I installed yesterday throws so many notices already.

I am just using request.js. And that seems like an odd user experience. Especially since nothing shows up in npm outdated?

@wolfy1339
Copy link
Member

Next major release will get that cleaned up

@iomarcovalente
Copy link

I guess that wasn't cleaned up in the "next major release" as I am still seeing this issue

@wolfy1339
Copy link
Member

This upcoming major release, which is in the works actually removes the deprecated code.

See #401

@till
Copy link
Contributor

till commented Feb 22, 2024

@wolfy1339 Do you want to reopen this ticket until it is fixed? 🫣

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

No branches or pull requests

5 participants