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

Stop exposing the URL polyfill in the global scope #9868

Merged
merged 1 commit into from
Jul 8, 2018

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Jul 3, 2018

This moves/exposes the URL polyfill similarily to the existing ReadableStream polyfill, rather than exposing it globally, to avoid interfering with any "outside" code.
Both the URL and ReadableStream polyfills are now exposed on the pdfjsLib object, such that they are accessible to the viewer components.
Furthermore, the no-restricted-globals ESLint rule is also enabled to prevent accidental usage of the native URL/ReadableStream implementations directly in the src/ and web/ folders; see also https://eslint.org/docs/rules/no-restricted-globals

Addresses the remaining TODO in https://github.com/mozilla/pdf.js/projects/6


By not exposing the polyfilled URL globally, the PR might also "fix" issues #8726 and #9358 for free.

/cc @yurydelendik

@pdfjsbot
Copy link

pdfjsbot commented Jul 3, 2018

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/4506105370e662a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 3, 2018

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/4506105370e662a/output.txt

Total script time: 8.11 mins

Published

This moves/exposes the `URL` polyfill similarily to the existing `ReadableStream` polyfill, rather than exposing it globally, to avoid interfering with any "outside" code.
Both the `URL` and `ReadableStream` polyfills are now exposed on the `pdfjsLib` object, such that they are accessible to the viewer components.
Furthermore, the `no-restricted-globals` ESLint rule is also enabled to prevent accidental usage of the native `URL`/`ReadableStream` implementations directly in the `src/` and `web/` folders; see also https://eslint.org/docs/rules/no-restricted-globals

Addresses the remaining TODO in https://github.com/mozilla/pdf.js/projects/6
@pdfjsbot
Copy link

pdfjsbot commented Jul 4, 2018

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/380e76e348e4a2f/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 4, 2018

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/380e76e348e4a2f/output.txt

Total script time: 7.70 mins

Published

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jul 4, 2018

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/488ec3ed977e84a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 4, 2018

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/9b0807b4ae001a1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 4, 2018

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/9b0807b4ae001a1/output.txt

Total script time: 24.03 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Jul 4, 2018

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/488ec3ed977e84a/output.txt

Total script time: 37.51 mins

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

@timvandermeij timvandermeij merged commit 3101257 into mozilla:master Jul 8, 2018
@timvandermeij
Copy link
Contributor

Nice work! It's good to not change the global scope anymore with the URL polyfill.

This was referenced Jul 8, 2018
@Snuffleupagus Snuffleupagus deleted the url-polyfill branch July 8, 2018 14:47
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Stop exposing the `URL` polyfill in the global scope
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