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

1.8.0 tests have 102 failures in IE8 #2062

Closed
paulfalgout opened this Issue Feb 20, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@paulfalgout
Copy link
Contributor

paulfalgout commented Feb 20, 2015

Not the best release.. not the worst

@L1fescape

This comment has been minimized.

Copy link

L1fescape commented Feb 20, 2015

👍 :shipit:

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Feb 20, 2015

Probably the worst.

@jashkenas jashkenas added the bug label Feb 20, 2015

@medcat

This comment has been minimized.

Copy link

medcat commented Feb 20, 2015

:shipit:

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Feb 20, 2015

Simply removing the hasEnumBug and collectNonEnumProps workarounds has the result of fixing the vast majority of these errors. Any bright ideas?

@paulfalgout

This comment has been minimized.

Copy link
Contributor

paulfalgout commented Feb 20, 2015

I haven't tried the test directly, but my app works if I remove toString from nonEnumerableProps

Just a start.

  var hasEnumBug = !{toString: null}.propertyIsEnumerable('toString');
  var nonEnumerableProps = ['constructor', 'valueOf', 'isPrototypeOf',
                      'propertyIsEnumerable', 'hasOwnProperty', 'toLocaleString'];
@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Feb 20, 2015

@paulfalgout — Please give the current master a try and let me know if it works for you in your app.

@jamiebuilds

This comment has been minimized.

Copy link
Contributor

jamiebuilds commented Feb 20, 2015

It might be good to fix some of these tests instead of deleting them.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Feb 20, 2015

@paulfalgout When you get a chance — I've pushed out 1.8.1, with changes that allow the important tests to pass in IE. Let us know how its working for you...

@jashkenas jashkenas added the fixed label Feb 20, 2015

@jashkenas jashkenas closed this Feb 20, 2015

@paulfalgout

This comment has been minimized.

Copy link
Contributor

paulfalgout commented Feb 20, 2015

Well 1.8.1 doesn't totally bomb, but I'm seeing a very strange bug now. Switching back to 1.6.0 it works fine, so I'm attempting to see if something around this changed in underscore or if there's a new issue.

Also debugging in IE8 is the worst.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Feb 20, 2015

Well, if you can only change Underscore, and see issues with 1.8.1, then it's probably our fault. Let us know when you have a failing test case — and we can push out a revised 1.8.2 ... or however many it takes to get it right.

@paulfalgout

This comment has been minimized.

Copy link
Contributor

paulfalgout commented Feb 20, 2015

Actually as far as my app is concerned I think I'm in the clear. The bug I was seeing was an older use of _.template passing data as the 2nd attribute. Didn't catch it earlier because we initially didn't upgrade to 1.7.0 because I have some legacy code that depends on 1.6.0 _.extend and it is unfortunately difficult to refactor. (though it will be refactored soon) Difficult bug to track down, because our current tests fail a bit after the template renders as "function(){ return render.call ..." in a nearly unrelated place. IE8 + contenteditable = 😿

Anyhow.. a bit nervous about everything that was pulled out at the last moment. I'm sure that'll be addressed in the near term. In the future I'm more than willing to run a preview branch through the ringer..as I'm sure others are as well. I've been watching this release fairly closely, but it seemed like it was a ways off.. and then it wasn't. But we could work on catching these things prior to a release.

I do appreciate the work and the library for sure, and there are many brilliant contributors, but I don't have a ton of confidence in many last minute changes. There's just no way I could sell using 1.8.1 at work without waiting a long time for possible issues to rise to the surface. We're pretty willing to live on the edge, but this just isn't safe enough.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Feb 20, 2015

Thanks for checking it out and reporting back. Living on the edge is never a good idea — you could always take the time to try out new versions of things.

For what it's worth, 1.6.0 _.extend is back in 1.8+ ... so you should be all set on that front.

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