-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Jade browser client .js does not work in IE7 or 8 #254
Comments
I have no VM set up to test IE unfortunately so I'm not much of a help |
Can you tell us what the error message you are seeing is? I'll probably help diagnose what's breaking. A filename/linenumber would be best... |
I think have a fix and will provide a diff to jade.js shortly. It was a combination of 2 things
Still testing to see if the parsing is correct, so far looks to be ok. |
There are quite a few libraries around that add missing functionality to older browsers (mainly IE7&8). They add functions like inArray, string.trim() etc... It might be worth testing Jade with some of these libraries and then just make it a dependency for older browsers. |
each enough to avoid using trim() etc |
Just FYI, this is the code I am using currently that seems to work in IE7 and IE8. http://173.255.240.43/js/jade.js?v=e4420831-0fb6-4523-a68f-1b9df637bec1 I'm not sure on next steps though on what I should do regarding this bug. |
It appears the following cases makes this work.
The code I posted in the previous comment does NOT work. I'm not sure why this is, but it appears that without something in prototype.js, the HTML output by Jade will not include classnames, attributes, IDs, etc... yet it will include content inside div tags. Any ideas? |
Could this be to do with the forEach function? If there is no forEach function then Jade cannot loop through each of the attributes and add them to the divs... |
It depends on the native forEach function being there IN ADDITION TO something in prototype.js (which I've yet to identify exactly what). Prototype.js provides Array.indexOf and Array.map, but it also provides something else that somehow makes the jade parse correctly. Otherwise jade will parse without classnames, ids, etc. |
it's not like we have to use forEach :p I can easily change that |
When I test running a precompiled template function in IE8, I get an error on the first line of code in the following. This code is within the
I'm guessing the problem is that Note that I'm not loading or running jade in the client. I'm sending precompiled templates as raw javascript from the server. The templates were compiled on the server and saved out as a string |
I agree that this needs to be addressed.... especially for cases similar to @tauren. IMO I dislike de-optimising code for all browsers just so it will work in IE, so would prefer to see a requirement that Array.map is implemented manually if not already supported. Either state that a shim script is required, or, as part of the rendering process in Jade, implement the missing functionality as required. |
I'd rather stay away from shims, you'll otherwise have bloat from libs all adding conditional shims that they need, or if we specify that you just need to add one as a dep it's an extra annoyance, we might as well just use a for loop, there's already a lot of ugly code due to client-side stuff anyway |
My suggestion would have been to add it as an extra dependency as I hate the fact that I constantly have to write unoptimised code so it will be compatible with IE. I agree that this is an extra annoyance but it is also becoming more common among client side libraries. In this case, replacing Array.map with a loop would be near trivial, but it also feels like defeat. |
using map etc isn't really optimizing, it's slower, not that speed is relevant in that chunk of code anyways, but I know what you mean, I really dislike catering to both environments. |
If you are using underscore, you can add this shim (CoffeeScript): unless Array.prototype.map?
Array.prototype.map = (callback, context) -> _.map(@, callback, context)
unless Object.keys?
Object.keys = _.keys Or if you haven't drank the coffee/cool-aid yet: if (Array.prototype.map == null) {
Array.prototype.map = function(callback, context) {
return _.map(this, callback, context);
};
}
if (Object.keys == null) {
Object.keys = _.keys;
} Like @tauren, I'm sending compiled templates down. I'm using Stitch: stitch.compilers.jade = (module, filename) ->
content = jade.compile fs.readFileSync(filename, 'utf8')
module._compile "module.exports = #{content};", filename A proper patch to avoid calls to these two methods would be nice. If someone would like to take a crack at it. I'm just gonna run with this shim for now. |
stitch requires you to write sync? |
what's the status of this? Would make sense to use mocha for tests and run them on different browsers? |
This issue has been inactive for over 2 months so I'm closing it. If you think it's still an issue re-open. - tjbot |
I'm having the exact issue @weixiyen mentioned about missing attributes in IE. I've tried adding missing functions for IE from this StackOverflow page but not getting anywhere. Did anyone get this working? |
For what it's worth, I believe that this is the cause: /**
* Lame Array.isArray() polyfill for now.
*/
if (!Array.isArray) {
Array.isArray = function(arr){
return '[object Array]' == toString.call(arr);
};
}
/**
* Lame Object.keys() polyfill for now.
*/
if (!Object.keys) {
Object.keys = function(obj){
var arr = [];
for (var key in obj) {
if (obj.hasOwnProperty(key)) {
arr.push(obj);
}
}
return arr;
}
} If I replace those polyfills with the following, everything seems to work: if (!Array.isArray) {
Array.isArray = function(arr) {
return _.isArray(arr);
};
}
if (!Object.keys) {
Object.keys = function(obj) {
return _.keys(obj);
};
} |
I didn't determine if one or both or these are required. |
we should get rid of both polyfills anyway, a lib isn't the right place to add those, when I have a sec I'll remove those and use utils instead of Object.keys() etc |
Is this still an issue? |
We're still using the polyfills I mentioned above. I'm assuming it's still an issue since this is still open. |
@paulyoung I've been going through and closing many of the old issues that turn out to have been fixed ages ago, I just wanted to confirm that this is still an issue and not just something nobody got round to closing. |
I believe this is still an issue as the "lame" polyfills still exist exactly as before. |
Yup I have encountered this as well. Should be a simple fix I shall look into it @ForbesLindesay |
Meh, this is a non-goal, just add the necessary polyfills for any missing functionality using something like |
Here's the complete set of polyfills that I used to make it work in IE8. No underscore required. |
Are there any plans to support IE7 and IE8?
The text was updated successfully, but these errors were encountered: