Remove SystemJS usage from the development viewer and the unit-tests #12527
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/cd3297cbd7c6178/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/cd3297cbd7c6178/output.txt Total script time: 3.98 mins Published |
|
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/a0cfe7c2310bddf/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/a0cfe7c2310bddf/output.txt Total script time: 3.41 mins
|
timvandermeij
approved these changes
Oct 25, 2020
…xport` statements While the *built* `pdf.worker.js` file still works correctly with these changes, despite these two files being excluded by Babel[1], the development viewer does not because of issues with SystemJS[2] and/or its Babel-plugin (both of which are old). Furthermore, note also that excluding these two files from Babel-processing isn't *generally* necessary since e.g. the `gulp mozcentral` command works anyway. The explanation is rather that it's actually the source-map generation which fails for these huge sequences when building the `pdf.worker.js` file. However, not using standard `import`/`export` statements in all files means we also need to use SystemJS when e.e. running the unit-tests. This is very unfortunate, since SystemJS (or its old Babel-version) doesn't support modern ECMAScript features such as e.g. optional chaining and nullish coalescing. Unfortunately it also seems that https://bugzilla.mozilla.org/show_bug.cgi?id=1247687, which tracks the implementation of worker-modules in Firefox, has stalled since there hasn't been any updates for six months now. To hopefully address all of the above, this patch is the first in a series that attempts to further reduce our reliance on SystemJS. --- [1] The only difference being how the dependencies are handled, in the Webpack-bundled file. [2] Parsing takes way too long and consumes too much memory, thus rendering the development viewer essentially unusable.
…ormat `src/core/{glyphlist, unicode}.js`
*Please note:* Once https://bugzilla.mozilla.org/show_bug.cgi?id=1247687 is implemented, and we've removed SystemJS completely, this entire patch can (and even should) be reverted.
This is similar to the existing `getLookupTableFactory` helper function, but is implemented as outlined in issue 6774.
The re-formatting of the tables were done automatically, by using find-and-replace with regular expressions.
For reasons that I don't even pretend to understand, using this particular structure for these *very* long lookup tables allow SystemJS to process the files correctly/quickly and the development viewer thus works as intended.
…ding of `pdf.worker.js`, when source-maps are enabled This produces a slightly smaller built `pdf.worker.js` file, for e.g. the `gulp mozcentral` build-target.
…port` This removes the last SystemJS usage from both the API and the default viewer.
Since we no longer use SystemJS to load the unit-tests, there's now nothing that prevents us from using optional chaining and nullish coalescing in the `src/display/` directory.
Snuffleupagus
force-pushed
the
worker-rm-require
branch
from
68d6c7c to
c293fc2
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/ae70f6cbfa48198/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/ae70f6cbfa48198/output.txt Total script time: 24.81 mins
Image differences available at: http://54.67.70.0:8877/ae70f6cbfa48198/reftest-analyzer.html#web=eq.log |
…hared/` folder Given that this code is used on the worker-thread, where SystemJS is still used during development, we need to (for now) handle this folder the same way as the `src/core/` one.
timvandermeij
approved these changes
Oct 26, 2020
|
Nice work! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please refer to the individual commit messages for additional details.
It's highly recommended to review this one commit at a time, perhaps also ignoring whitespace-only changes where appropriate.
The second commit is probably more-or-less impossible to actually review, so we're probably going to have to trust our test-suite and that I wrote proper regular expressions.
This still leaves SystemJS usage in the font-tests, however fixing that will require larger changes and it thus seemed best to defer that to later since this PR is already very large.