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

Add support for async/await using Babel #9977

Merged
merged 1 commit into from
Aug 21, 2018

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Aug 13, 2018

For proof-of-concept, this patch converts a couple of Promise returning methods to use async instead.
Please note that the generic build, based on this patch, has been successfully testing in IE11 (i.e. the viewer loads and nothing is obviously broken).

Being able to use modern JavaScript features like async/await is a huge plus, but there's one (obvious) side-effect: The size of the built files will increase slightly (unless SKIP_BABEL == true). That's unavoidable, but seems like a small price to pay in the grand scheme of things.

Finally, note that the chromium build target was changed to no longer skip Babel translation, since the Chrome extension still supports version 49 of the browser (where native async support isn't available).

Split off from PR #9944 as requested by @brendandahl.

Much more manageable diff with https://github.com/mozilla/pdf.js/pull/9977/files?w=1

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/fe7ccce125af56e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/404b269a2dc9dec/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/404b269a2dc9dec/output.txt

Total script time: 29.35 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/404b269a2dc9dec/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/fe7ccce125af56e/output.txt

Total script time: 36.36 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

The one Windows failure is strange, since the commit included here is exactly the same as the first one in PR #9944 (where all tests ran successfully). One possible explanation is that Firefox was updated on the bot (which might not show up in the logs, since the bot uses a beta version); another explanation is that it's simply an intermittent.

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/57b06f2b255b115/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/57b06f2b255b115/output.txt

Total script time: 29.29 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/57b06f2b255b115/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/1fe25d302a82c2f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/1fe25d302a82c2f/output.txt

Total script time: 29.16 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/1fe25d302a82c2f/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/54ef6786685e78d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/54ef6786685e78d/output.txt

Total script time: 29.80 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/54ef6786685e78d/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

I don't understand what to make of these intermittent failures (on Windows), since all tests seem to pass consistently in PR #9944 and the commit hashes are identical 😕

@brendandahl
Copy link
Contributor

Doesn't look like it's just your changes, I got it to fail over in #9567 (comment)

@timvandermeij
Copy link
Contributor

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 1

Live output at: http://54.215.176.217:8877/e84f0b739e5d438/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/e84f0b739e5d438/output.txt

Total script time: 30.35 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

return this.CMapReaderFactory.fetch({
name: data.name,
});
return this.CMapReaderFactory.fetch(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, this was unrelated to the main objective of the patch; it's been reverted now.

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/95b758af0bed3c8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/95b758af0bed3c8/output.txt

Total script time: 38.70 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

For proof-of-concept, this patch converts a couple of `Promise` returning methods to use `async` instead.
Please note that the `generic` build, based on this patch, has been successfully testing in IE11 (i.e. the viewer loads and nothing is obviously broken).

Being able to use modern JavaScript features like `async`/`await` is a huge plus, but there's one (obvious) side-effect: The size of the built files will increase slightly (unless `SKIP_BABEL == true`). That's unavoidable, but seems like a small price to pay in the grand scheme of things.

Finally, note that the `chromium` build target was changed to no longer skip Babel translation, since the Chrome extension still supports version `49` of the browser (where native `async` support isn't available).
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/bdc5647df240180/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/025a6c0395cbc0d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/025a6c0395cbc0d/output.txt

Total script time: 31.36 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/025a6c0395cbc0d/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/bdc5647df240180/output.txt

Total script time: 36.06 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@brendandahl brendandahl merged commit 20cd1b3 into mozilla:master Aug 21, 2018
@Snuffleupagus Snuffleupagus deleted the async-src branch August 21, 2018 09:29
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.

4 participants