search

es-shims / es5-shim

ECMAScript 5 compatibility shims for legacy (and modern) JavaScript engines
MIT License
7.12k stars 895 forks source link

handle no `deleteCount` to `splice()` in Opera #465

Closed tjenkinson closed 4 years ago

tjenkinson commented 5 years ago

In Opera 10.6 and 9.8, when the fix is applied and deleteCount is missing, no elements are deleted. This is incorrect - when deleteCount is missing the behavior should be to delete all elements from start onwards

tjenkinson commented 5 years ago

I can find out the version on Monday when I have access to the device again.

How should I test this? We need to force adding the implementation, which I didn't see done anywhere else

FYI I opened a related PR on typescript here: https://github.com/microsoft/TypeScript/pull/32643

ljharb commented 5 years ago

I'm not sure what you mean by "force adding the implementation" - splice is force-replaced here, here, and here.

tjenkinson commented 5 years ago

I mean we want a test that tests

https://github.com/es-shims/es5-shim/blob/e11b3f7be2ca1e87399478ba02553e110e5759fa/es5-shim.js#L819-L880

But that code only executes based on some of the other conditions

tjenkinson commented 5 years ago

It's Opera 9.8

ljharb commented 5 years ago

@tjenkinson thanks!

As for a test, anything that invokes .splice and omits deleteCount seems like it would test this change?

tjenkinson commented 5 years ago

But it would only test the change on something that has one of these issues right?

https://github.com/es-shims/es5-shim/blob/e11b3f7be2ca1e87399478ba02553e110e5759fa/es5-shim.js#L881

Would the tests run on a browser old enough to have these issues?

ljharb commented 5 years ago

CI won't, but I can manually run them against that version of Opera :-)

tjenkinson commented 5 years ago

https://github.com/es-shims/es5-shim/blob/e11b3f7be2ca1e87399478ba02553e110e5759fa/tests/spec/s-array.js#L1364-L1370

:D

tjenkinson commented 4 years ago

hi @ljharb any chance this could be merged soon?

ljharb commented 4 years ago

Hmm, looks like the oldest Opera I can test in is 10.6, but the test seems to fail there.

ljharb commented 4 years ago

It seems tho, like your fix ends up causing 2 additional failures that weren't there before.

tjenkinson commented 4 years ago

:/ Which tests failed for you?

ljharb commented 4 years ago 
should return right result 3
Expected [ '~0', '~ 1', '~ 2', 'F1', 'P', 'LLL', 'CCC', 'YYY', 'XXX', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 1, 23, 4, 5, 6, 7, 8, '~ 4', '~ 5', '~ 6', '~ 7', 7, 8, 9, 10, 11, 2, 4, 5, 6, 7, 8, 9, 'CCC', 'YYY', 'XXX', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 1, 23, 4, 10, 1, 2, 3, 4, 5, 6, 7, 8, 9, 1, 2, 3 ] to equal [ '~0', '~ 1', '~ 2', 'F1', 'P', 'LLL', 'CCC', 'YYY', 'XXX', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 1, 23, 4, 5, 6, 7, 8, '~ 4', '~ 5', '~ 6', '~ 7', 7, 8, 9, 10, 11, 2, 4, 5, 6, 7, 8, 9, 'CCC', 'YYY', 'XXX', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 1, 23, 4, 9, 10, 1, 2, 3, 4, 5, 6, 7, 8, 9, 'YYY', 'XXX', 'F6', 'F7', 'F8', 'F9', 'F10', 'F11', 'F12', 'F13', 'F14', 'F15', 'F16', 'F17', 'F18', 'F19', 'F20', 'F21', 'F22', 'F23', 'F24', 'F25', 'F26', 3, 4, 5, 6, 7, 8, 9, '-0', '- 1', '- 2', '- 3', '- 4', '- 5', '- 6', '- 7', 1, 2, 3 ].
run
should do nothing if method called with no arguments
Expected [ 1, 'a', [ 'b' ] ] to equal [ ].
Expected [ ] to equal [ 1, 'a', [ 'b' ] ].
ljharb commented 4 years ago

@tjenkinson i know it's been awhile, but is there any chance you're interested in completing this PR?

tjenkinson commented 4 years ago

@ljharb I am, but I'm not sure when I'm going to get time at the moment. Will close for now and maybe reopen later

ljharb commented 4 years ago

I'd rather keep it open in the meantime so I can track it :-) please return to it when you get time.

tjenkinson commented 4 years ago

@ljharb I am seeing a lot of failures like this, even on master

  32) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  33) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  34) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  35) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  36) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  37) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

  38) Date #getSeconds() should return the right value for negative dates
   Message:
     Expected 55 to be 59.
   Stacktrace:
     Error: Expected 55 to be 59.
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:469:47
    at Array.forEach (<anonymous>)
    at /Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:468:28
    at Array.forEach (<anonymous>)
    at jasmine.Spec.<anonymous> (/Users/tomjenkinson/Documents/GitHub/es5-shim/tests/spec/s-date.js:467:26)

any idea why?

ljharb commented 4 years ago

In Opera specifically? Which version, I can probably fix those.

tjenkinson commented 4 years ago

This is just from running npm test on e938fda

ljharb commented 4 years ago

On which node version? travis-ci passes on a great many node versions: https://travis-ci.com/es-shims/es5-shim/builds/143354113

tjenkinson commented 4 years ago

v11.13.0

ljharb commented 4 years ago

https://travis-ci.com/es-shims/es5-shim/jobs/272983149 passes, i'll try locally.

tjenkinson commented 4 years ago

Weird. I wonder if it's related to my timezone somehow? I'm GMT+0100 (Central European Standard Time) at the moment

ljharb commented 4 years ago

interesting, i see those failures locally too, and i'm in UTC-7

tjenkinson commented 4 years ago

Ah when I change my timezone to London I get Error: Expected 44 to be 59., so it looks like it's having an effect

tjenkinson commented 4 years ago

hopefully with the last few commits the tests will be fixed now 🤞

ljharb commented 4 years ago

It appears that old v8, old spidermonkey, jsc, and xs all print out 59 for this test; new v8, new spidermonkey, and chakra all print out 1, so i'm looking into whether it's a spec change, or a browser bug.

tjenkinson commented 4 years ago

Are you referring to the date test or splice?

ljharb commented 4 years ago

The date test :-) i'll take a look at your splice changes soon.

tjenkinson commented 4 years ago

bump!

ljharb commented 4 years ago

OK, with your test but without your fix, Opera 10.6 has 10 failures; with your fix, it has 8, none of which are related to splice. Let's get this in :-)

tjenkinson commented 4 years ago

:tada: thanks