Skip to content

Unify method return values in the ObjectLoader class, and simplify how the ObjectLoader is used #19895

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

Merged
merged 2 commits into from
May 6, 2025

Conversation

Snuffleupagus
Copy link
Collaborator

  • Unify method return values in the ObjectLoader class

    Given that all the methods are already asynchronous we can just use await more throughout this code, rather than having to explicitly return function-calls and undefined.
    Note also how none of the ObjectLoader.prototype.load call-sites use the return value.

  • Simplify how the ObjectLoader is used

    The ObjectLoader.prototype.load method has a fast-path, which avoids any lookup/parsing if the entire PDF document is already loaded.
    However, we still need to create an ObjectLoader-instance which seems unnecessary in that case.

    Hence we introduce a static ObjectLoader.load method, which will help avoid creating ObjectLoader-instances needlessly and also (slightly) shortens the call-sites.
    To ensure that the new method will be used, we extend the no-restricted-syntax ESLint rule to "forbid" direct usage of new ObjectLoader().

Given that all the methods are already asynchronous we can just use `await` more throughout this code, rather than having to explicitly return function-calls and `undefined`.
Note also how none of the `ObjectLoader.prototype.load` call-sites use the return value.
The `ObjectLoader.prototype.load` method has a fast-path, which avoids any lookup/parsing if the entire PDF document is already loaded.
However, we still need to create an `ObjectLoader`-instance which seems unnecessary in that case.

Hence we introduce a *static* `ObjectLoader.load` method, which will help avoid creating `ObjectLoader`-instances needlessly and also (slightly) shortens the call-sites.
To ensure that the new method will be used, we extend the `no-restricted-syntax` ESLint rule to "forbid" direct usage of `new ObjectLoader()`.
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@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/c79511bcd031d90/output.txt

@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/93452580a8aa58d/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 29.77 mins

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/93452580a8aa58d/output.txt

Total script time: 61.78 mins

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

@Snuffleupagus Snuffleupagus changed the title Unify method return values in the ObjectLoader class, Simplify how the ObjectLoader is used Unify method return values in the ObjectLoader class, and simplify how the ObjectLoader is used May 6, 2025
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@Snuffleupagus Snuffleupagus merged commit 6f05231 into mozilla:master May 6, 2025
9 checks passed
@Snuffleupagus Snuffleupagus deleted the ObjectLoader-improve branch May 6, 2025 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants