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

Enable IE 11 compability of Angular + pdf.js #8192

Closed
Wolfke85 opened this issue Mar 24, 2017 · 10 comments
Closed

Enable IE 11 compability of Angular + pdf.js #8192

Wolfke85 opened this issue Mar 24, 2017 · 10 comments
Labels

Comments

@Wolfke85
Copy link

Wolfke85 commented Mar 24, 2017

Configuration:

  • Web browser and its version: IE 11
  • Operating system and its version: Any
  • PDF.js version: 1.4.0
  • AngularJs version: 1.5.11

Steps to reproduce the problem:

  1. Use AngularJS 1.5.11 with automatic bootstrapping
  2. Include PDF.js version 1.4.0 BEFORE the script tags of Angular

What is the expected behavior? (add screenshot)

  • Angular works perfectly together with PDF.js
  • PDF.js wraps missing API in a functions that will use the shimmed version if the native one is not available.

What went wrong? (add screenshot)

  • See Enable IE 11 compability of Angular + pdf.js angular/angular.js#15772
    Currently pdf.js defines document.currentScript but not link.origin nor link.protocol. If angular starts it checks if it is allowed to bootstrap automatically it checks for currentScript and asume that this will be enough to filter IE, meaning if currentScript is not defined we can automatically bootstrap. This check will not work in combination with pdf.js.

Libraries shouldn't modify properties of objects they don't own as it causes issues like this. If they need a particular API that is missing in some supported environments they may wrap its usage in a function that will use the shimmed version if the native one is not available.

Link to a viewer (if hosted on a site other than mozilla.github.io/pdf.js or as Firefox/Chrome extension):
http://plnkr.co/edit/YFCQM2Px0QU0KnGzsAlM?p=preview

@yurydelendik
Copy link
Contributor

yurydelendik commented Mar 24, 2017

Sounds like shifting blame from somewhat IE11-incomplete security check Angular logic to PDF.js polyfill... alright. Marking as a simple bug to fix this gap:

  1. document.location.origin must be set to new URL(document.location.href).origin if 'origin' property is absent
  2. HTMLScriptElement.prototype shall have the origin and protocol getters with the similar logic as above.

@mgol
Copy link
Contributor

mgol commented Mar 24, 2017

@yurydelendik For me this is analogous to the situation where a couple of Web APIs had to change their names because MooTools was applying partial polyfills and code on the Web started depending on it.

Polyfilling is risky, IMO it should be done only with complete polyfills and only in final apps, not in libraries. If libraries need a particular API that's not available everywhere, they should wrap the native API in an utility function and use that function; that removes the whole class of possible bugs like this one.

In short: library code shouldn't modify objects it doesn't own, e.g. native browser globals.

@yurydelendik
Copy link
Contributor

yurydelendik commented Mar 24, 2017

Polyfilling is risky

@mgol oh, I agree. Entire PDF.js viewer application must reside in its own sandbox (I recommend iframe), but people keep using it within bigger apps. So we have to react accordingly.

@yurydelendik
Copy link
Contributor

library code shouldn't modify objects it doesn't own, e.g. native browser globals.

Let's take another example, without typed arrays and Promise PDF.js library would not be the same. And by not modifying global objects we would make our code unreadable and probably less performant for majority of the modern browsers.

@mgol
Copy link
Contributor

mgol commented Mar 24, 2017

Let's take another example, without typed arrays and Promise PDF.js library would not be the same. And by not modifying global objects we would make our code unreadable and probably less performant for majority of the modern browsers.

Not necessarily. You could keep your own internal set of variables that shadow globals. This may not cover all cases but at least promises should be fine. Something like:

var Promise = window.Promise || PROMISE_POLYFILL;

at the top of PDF.js. In that case you're not touching the global and you still can use Promise in your code without any changes.

This shouldn't affect performance in modern browsers either as it's a simple alias for them.

I understand it may not be so easy in all the cases, though (e.g. if only a few methods need to be polyfilled)

@yurydelendik
Copy link
Contributor

PDF.js code is transforming to use ES6 modules. The above approach can be problematic atm, unless a packager will automatically provide a polyfill.

I would like not turn this issue into discussion about angular logic or pdf.js compatibility approach, it will be nice to fix a our polyfill so we will play nice with angular. A PR is welcome :)

@yurydelendik
Copy link
Contributor

On the different thought, let's close this issue as won't fix. There is no confirmation that there are Angular+PDF.js+IE11 real world uses, if there is, the fix can be easy made on angular side by patching Angular.js.

@elliotstoner
Copy link

Hey there, I know this is an old topic but I'm running into the issue described above.

I have a project where I'm using Angular 5 with PDF.js and I'm required to support IE11. I'm a newbie to Angular so I'm not exactly sure how I would "patch Angular.js" - can you give me some guidance as to how I can get around this defect? Thanks in advance.

@mgol
Copy link
Contributor

mgol commented Aug 20, 2018

@elliotstoner this issue is about AngularJS compatibility, not Angular (2+) and only applies if you use ng-app instead of manual bootstrapping. Angular 2+ doesn't even support automatic bootstrapping so this issue won't apply there.

@elliotstoner
Copy link

@mgol Ah ok - I'll create a new issue then, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants