[api-minor] Replace various getAll methods with iterators
#19778
Conversation
Snuffleupagus
force-pushed
the
replace-getAll
branch
from
7cf1c8a to
850dcb7
Compare
Snuffleupagus
force-pushed
the
replace-getAll
branch
from
850dcb7 to
9f40dfd
Compare
These `getAll` methods are not used anywhere within the PDF.js code-base, outside of tests, and were mostly added (speculatively) for third-party users. To still allow access to the same data we instead introduce iterators on these classes, which (slightly) shortens the code and allows us to remove the `objectFromMap` helper function. A summary of the changes in this patch: - Replace the `getAll` methods with iterators in the following classes: `AnnotationStorage`, `Metadata`, and `OptionalContentGroup`. - Change, and also re-name, `AnnotationStorage.prototype.setAll` into a test-only method since it's not used elsewhere. - Remove the `Metadata.prototype.has` method, since it's only used in tests and can be trivially replaced by calling `Metadata.prototype.get` and checking if the returned value is `null`.
Snuffleupagus
force-pushed
the
replace-getAll
branch
from
9f40dfd to
2c593b0
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/e9ae91cbee2b16e/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/ba1cf9c8fc9f781/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/e9ae91cbee2b16e/output.txt Total script time: 29.89 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/ba1cf9c8fc9f781/output.txt Total script time: 62.12 mins
|
|
Question: how do you define api-minor and api-major? This PR is removing a method that you describe as "mostly used by consumers of the public API", which seems like the exact definition of an API breaking change. |
Well, that's not exactly what the commit message says though: "[...] and were mostly added (speculatively) for third-party users." It's impossible to know if users are actually depending on this, and for e.g. |
These
getAllmethods are not used anywhere within the PDF.js code-base, outside of tests, and were mostly added (speculatively) for third-party users.To still allow access to the same data we instead introduce iterators on these classes, which (slightly) shortens the code and allows us to remove the
objectFromMaphelper function.A summary of the changes in this patch:
Replace the
getAllmethods with iterators in the following classes:AnnotationStorage,Metadata, andOptionalContentGroup.Change, and also re-name,
AnnotationStorage.prototype.setAllinto a test-only method since it's not used elsewhere.Remove the
Metadata.prototype.hasmethod, since it's only used in tests and can be trivially replaced by callingMetadata.prototype.getand checking if the returned value isnull.