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

Using on modern browsers while not breaking non-supported browsers #112

Closed
mikeebee opened this issue Feb 15, 2016 · 26 comments
Closed

Using on modern browsers while not breaking non-supported browsers #112

mikeebee opened this issue Feb 15, 2016 · 26 comments

Comments

@mikeebee
Copy link

Hi there, I know that Ally.js doesn't support IE9 and below (currently) but could you advise how I could safely use this on a project that supports IE8 without it causing an blocking error?

The following worked in removing errors from IE9 but IE8 still gets this error: expected identifier, string or number

var IE9 = ($('.lt-ie10').length) ? true : false;

if (!IE9) {
    // Ally.js doesn't support IE9 yet
    var ally = require('ally.js');
}

This is obviously pretty crude, if you have a clever workaround it would be much appreciated. The project uses Browserify and Babel.

Many thanks

@rodneyrehm
Copy link
Member

Ally is going to support IE9 with the next official release. You can try using 1.1.0-beta.4 for now.

As for IE8 I have not yet investigated what we could/should do. I hoped adding html5-shiv would solve most of the problems, but I haven't actually tried running ally.js on IE8.


Depending on the tool you're using to resolve that require('ally.js') the code may end up in the built file anyway - and that's what's tripping IE8, I assume.

@rodneyrehm
Copy link
Member

I hoped adding html5-shiv would solve most of the problems […]

of course I meant the es5-shim.

I just had a quick look at intern and found that the test framework would not run on IE8.

As for simply loading but not using the library on IE8, I've found a few instances where SVGElement is accessed, which is unknown to IE8 and thus throws errors. After fixing those and loading es5-shim and es5-sham.js, I'm getting an error-free load of the page:

<!--[if lt IE 9]>
  <script src="node_modules/es5-shim/es5-shim.js"></script>
  <script src="node_modules/es5-shim/es5-sham.js"></script>
<![endif]-->
<script src="dist/ally.dist.js"></script>
<script>console.log('loaded');</script>

I have not verified functional integrity, because the test framework won't run on IE8 - which would be the next step for #71

@rodneyrehm
Copy link
Member

It turns out that IE8 is no longer supported by the test framework. There used to be a separate version supporting older IE, but in light of Microsoft officially ending support for IE8 they ended support as well.

To be honest, I have no intention of fully supporting IE8. If it were as simple as supporting IE9 was, I'd bite the bullet and add support. But it's not. IE8 is way too far behind. I'm not yet sure how I want to proceed with this.

@rodneyrehm
Copy link
Member

This is obviously pretty crude, if you have a clever workaround it would be much appreciated. The project uses Browserify and Babel.

Did you configure anything so that you'd get ES3 code, or are you loading the es5-shim in that project?

@mikeebee
Copy link
Author

Thanks for such a detailed response!

I wouldn't expect you to support IE8, it's a shame I have to for this project. I guess I'm just looking for a workaround, so I can use it in supported browsers.

I do have the es5-shim (and sham) on this project. It seems that, if I run Ally through Babel (I had to turn compact to false to stop getting a file size error). I can remove the error from IE8. That's great but I could do without having to run a file of that size through Babel, plus, I'm unsure of the implications of turning that compact setting off.

I'm leaning towards maybe just injecting the script if it's not IE8.

    if (window.ally === undefined) {
        $('body').append('<script src="/js/src/plugins/ally.min.js"></script>');
    }

This seems to work ok, do you see an issue with that approach?

@mikeebee
Copy link
Author

I must have had errors in my code as I'm still getting the SVGElement issue in IE8 that you mentioned. So IE9 support is in the beta but the IE8 fix is in master? If that's correct, I'm happy to extract and build it myself.

@rodneyrehm
Copy link
Member

So IE9 support is in the beta but the IE8 fix is in master?

correct.

If that's correct, I'm happy to extract and build it myself.

You may want to try ally.js.tar.gz build 404 first. (just note that the file is not going to be available forever)

@mikeebee
Copy link
Author

I just tried the minified file in build 404 and I'm getting an error in IE8

Object doesn't support this property or method

from this line...

return y = c["default"](), document.addEventListener("shadow-focus", o, !0), document.documentElement.addEventListener(v, o, !0), document.documentElement.addEventListener(g, o, !0), h = m["default"](), o({ type: "initial" }), { used: a, current: u, lock: i };

Should I try the manual extraction instead?

@rodneyrehm
Copy link
Member

Object doesn't support this property or method

are you executing the code in IE8? Because that shouldn't work anyway.

loading ally.min.js (and es5-shim and es5-sham) I get

Message: Expected identifier, string or number
Line: 2
Char: 811
Code: 0
URI: http://test.dev/ally-ie8/node_modules/ally.js/ally.min.js

and that points to {default:e}. And I just realized we're running uglify with --screw-ie8, so yay…

Should I try the manual extraction instead?

sure, give it a go.

@mikeebee
Copy link
Author

It's getting Bablified and ShimShamed (new word!) so that's weird.

I tried doing build from the pre-release and got this...

npm ERR! Failed at the ally.js@1.1.0-beta.4 build script 'npm-run-all --sequential build:pre --parallel build:umd build:common build:amd --sequential build:post build:archive'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the ally.js package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm-run-all --sequential build:pre --parallel build:umd build:common build:amd --sequential build:post build:archive

npm v3.4.1
node v4.2.1

Any ideas?

@rodneyrehm
Copy link
Member

I tried doing build from the pre-release and got this...

what do you mean by pre-relase? Are you referring to Releasing ally.js?

npm run clean
npm run build

I'm running node v4.2.2 so that should be fine. But I'm on npm v2.14.7 - maybe that's a problem?

@mikeebee
Copy link
Author

I mean, I was using this 1.1.0-beta.4 pre-release build as my starting point.

Npm version could be an issue, not sure. I'm not super clued-up on npm stuff.

Do you have any rough gauge on when the IE9 support will make it into master? This project doesn't go live for a few weeks so I might just hold off.

@mikeebee
Copy link
Author

mikeebee commented Apr 8, 2016

Hi @rodneyrehm, after updating to v.1.1.0 I'm still getting Object doesn't support this method or property in IE8 on this line

var t=document.createElementNS("http://www.w3.org/2000/svg","svg");"classList"in t&&!window.QUnit||Object.defineProperty(SVGElement.prototype,"classList",{get:function(){return"string"==typeof this.className?new e(this,"className"):"string"==typeof this.className.baseVal?new e(this.className,"baseVal"):void 0}})}(),"undefined"!=typeof window&&(window.DOMTokenList=e),e})}

Any ideas?

@rodneyrehm
Copy link
Member

Looks like domtokenlist-shim is the problem here. I've created jwilsson/domtokenlist#16 to fix that.

@mikeebee
Copy link
Author

mikeebee commented Apr 9, 2016

Thanks!

@mikeebee
Copy link
Author

I notice that your domtokenlist-shim fix was merged in, do you need to update your fork?

@rodneyrehm
Copy link
Member

Sorry for the delay. I've pulled that IE8 fix into the fork. I didn't do this right away because I was working on something to replace domtokenlist-shim with, but then life happened. Anyway, the fix is in but now BrowserStack is acting up again. hoping to resolve this over the weekend…

@mikeebee
Copy link
Author

Hey no worries at all! I really appreciate you looking into this.

I tried downloading master and building the files and I'm still getting the error. Not sure if there is something else I needed to do.

@rodneyrehm
Copy link
Member

The build 536 contains the changes, but it's still failing, because I'm an idiot. I pulled in the fix to my fork, but did not update the distributables…

I've fixed the thing in build 538.

@mikeebee
Copy link
Author

Grabbed the ally.min.js file from build 538, loads of changes but I'm still getting that error.

@rodneyrehm
Copy link
Member

This is getting ridiculous. Apparently I didn't update all the distributables of domtokenlist-shim… the build 539 contains all the changes and I've verified loading that file (with es5-shim) in IE8.

@mikeebee
Copy link
Author

Heh no worries! Unfortunately, I'm getting a different error now. Same error message Object doesn't support... but now from this.

return y=d["default"](),document.addEventListener("shadow-focus",r,!0),document.documentElement.addEventListener(v,r,!0),document.documentElement.addEventListener(g,r,!0),h=m["default"](),r({type:"initial"}),{used:a,current:o,lock:i}}Object.defineProperty(n,"__esModule",{value:!0}),e("domtokenlist-shim/dist/domtokenlist-polyfill-umd");

Just to clarify, I'm using the ally.min.js file from the build folder you last sent. I have both these scripts on the page...

<script src="http://cdnjs.cloudflare.com/ajax/libs/es5-shim/4.5.8/es5-shim.min.js"></script>
<script src="http://cdnjs.cloudflare.com/ajax/libs/es5-shim/4.5.8/es5-sham.min.js"></script>

...and I am running the full file through Babel (although I've tried it without Babel and I get the same error).

@rodneyrehm
Copy link
Member

Can you check if ally-ie8 works for you? As far as I see I tested against es5-shim 4.5.7.

@mikeebee
Copy link
Author

Ok we're getting closer! That works fine, I also tested in my environment using the exact same scripts you are using and it works fine.

If I add part of the script I'm using, the error appears...

var handle = ally.style.focusSource();
handle.current(); // 'key', 'pointer', 'script'

Hope that helps

@rodneyrehm
Copy link
Member

Ha! ally.js will load in IE8, but it will not execute. The idea is that you don't have to serve different files to IE8 than to all the other browsers. But actually using ally.js in IE8 is not possible and will not be possible. You'll have to add escape hatches to your code so that in IE8 ally is not used.

@mikeebee
Copy link
Author

Aaaaah of course! My mistake, I was thinking (wrongly) that I would be able to use it without escaping for IE8. Added those in and everything is fine.

Thank you so much for nutting this out with me, you've gone above and beyond the call of duty to help me out and I really appreciate it.

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

No branches or pull requests

2 participants