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

[api-major] Replace MissingPDFException and UnexpectedResponseException with one exception #19264

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Snuffleupagus
Copy link
Collaborator

These old exceptions have a fair amount of overlap given how/where they are being used, which is likely because they were introduced at different points in time, hence we can shorten and simplify the code by replacing them with a more general ResponseException instead.

Besides an error message, the new ResponseException instances also include:

  • A numeric status field containing the server response status, similar to the old UnexpectedResponseException.

  • A boolean missing field, to allow easily detecting the situations where MissingPDFException was previously thrown.

@Snuffleupagus Snuffleupagus marked this pull request as ready for review December 29, 2024 11:21
@timvandermeij
Copy link
Contributor

timvandermeij commented Dec 29, 2024

Given that this is part of the public API, and I can imagine that third-party implementations actually handle the current exceptions, should we perhaps classify this as [api-major] instead since it can be seen as a breaking change?

@Snuffleupagus Snuffleupagus changed the title [api-minor] Replace MissingPDFException and UnexpectedResponseException with one exception [api-major] Replace MissingPDFException and UnexpectedResponseException with one exception Dec 29, 2024
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Dec 29, 2024

Given that this is part of the public API, and I can imagine that third-party implementations actually handle the current exceptions, should we perhaps classify this as [api-major] instead since it can be seen as a breaking change?

I suppose that's a fair comment; I've updated the commit message and PR title.

It's been a while since we bumped the major version number, and there's also a few things in progress (e.g. canvas tiling) that are significant enough to perhaps warrant a major version bump.
Maybe we should create one last release from the 4 branch, before landing this PR?

@Snuffleupagus Snuffleupagus force-pushed the ResponseException branch 4 times, most recently from eda9788 to 9f49568 Compare January 5, 2025 11:53
@mozilla mozilla deleted a comment from moz-tools-bot Jan 5, 2025
@mozilla mozilla deleted a comment from moz-tools-bot Jan 5, 2025
@mozilla mozilla deleted a comment from moz-tools-bot Jan 5, 2025
@mozilla mozilla deleted a comment from moz-tools-bot Jan 5, 2025
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/328150fc0c5c6f4/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/f562810b159cb5b/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/f562810b159cb5b/output.txt

Total script time: 28.19 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/328150fc0c5c6f4/output.txt

Total script time: 51.90 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: Passed

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, with one comment below. I do wonder if we want to wait with merging this until everything that should become part of PDF.js 5.x is identified so that in the meantime we can still make releases in the 4.x series if we need/want to?

test/unit/network_spec.js Show resolved Hide resolved
…ption` with one exception

These old exceptions have a fair amount of overlap given how/where they are being used, which is likely because they were introduced at different points in time, hence we can shorten and simplify the code by replacing them with a more general `ResponseException` instead.

Besides an error message, the new `ResponseException` instances also include:
 - A numeric `status` field containing the server response status, similar to the old `UnexpectedResponseException`.

 - A boolean `missing` field, to allow easily detecting the situations where `MissingPDFException` was previously thrown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants