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.

Sign up for GitHub

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

Jump to bottom

Handle API warnings #8

Closed
lucaswerkmeister opened this issue Nov 8, 2021 · 4 comments
Closed

Handle API warnings #8

lucaswerkmeister opened this issue Nov 8, 2021 · 4 comments
Assignees
@lucaswerkmeister
Labels
enhancement New feature or request
Milestone
1.0.0 release

Comments

@lucaswerkmeister
Copy link
Owner

Currently, we handle API errors, but not warnings. We should do better (putting my money where my mouth is: https://phabricator.wikimedia.org/T295215#7489064).
@lucaswerkmeister lucaswerkmeister added the enhancement New feature or request label Nov 8, 2021
@lucaswerkmeister lucaswerkmeister added this to the 1.0.0 release milestone Nov 8, 2021
@lucaswerkmeister
Copy link
Owner Author

For the record, I already have some local code for this, but I haven’t pushed it yet. I can probably do it soon, though.

@lucaswerkmeister lucaswerkmeister self-assigned this Nov 8, 2021
@lucaswerkmeister
Copy link
Owner Author

Hm, I’m not yet sure how this should interact with combining requests. When two requests with the same warn option are combined into one, and the response has warnings, should warn() be called once or twice?

And does the answer to that change when we add additional context to errors and warnings, such as the parameters for the request? What does that look like for combined errors/warnings, anyways?

@lucaswerkmeister
Copy link
Owner Author

I think for now I’ll have warn() be called twice, as if the requests hadn’t been combined. (Though the warnings could only apply to some of the original requests, of course.)

lucaswerkmeister added a commit that referenced this issue Nov 8, 2021
@lucaswerkmeister
Add support for handling API warnings
1298970 
Introduces a new ApiWarnings class, like ApiErrors. (No common
superclass, there’s not enough common code for it to be worth sharing in
my opinion.) This one’s not thrown, but having a stack trace can still
be useful. The default warning handler is console.warn in the browser
(should be fine, users don’t usually look there), while in Node.js it’s
only console.warn if NODE_ENV is explicitly set to “development”,
otherwise Node.js sends warnings into the void by default. README.md and
CHANGES.md encourage users to configure console.warn as the handler
unconditionally, but I don’t want to risk breaking CLI applications by
sending warnings to the console by default.

combine.js already needs to implement custom support for this option
(JSON.stringify() silently discards functions, so the previous code
would’ve ignored different warn functions), so I figured we might as
well implement the proper thing in this commit, where we can actually
combine requests with different warn options, just calling each
request’s warning handler with the same warnings object. Include some
tests for that as well.

Implements #8.
@lucaswerkmeister
Copy link
Owner Author

lucaswerkmeister commented Nov 8, 2021 •
edited
Loading

Done in 1298970. We can still tweak it before the 1.0.0 release.

@lucaswerkmeister lucaswerkmeister mentioned this issue Nov 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lucaswerkmeister lucaswerkmeister

Labels
enhancement New feature or request
Projects
None yet
Milestone
1.0.0 release
Development

No branches or pull requests
1 participant
@lucaswerkmeister