Safely call defineProperty when setting array length.
#273
Conversation
Older browsers, such as Firefox 22 and below, throw `new InternalError("defining the length property on an array is not currently supported")` when trying to set the length with `defineProperty` so a try catch will allow for those that do support it to use it, while those that don't fail due to the error.
wrap setting array length in try catch
src/9.negotiation.js
Outdated
| // "Freeze" the array so no new elements can be added | ||
| defineProperty(subset, 'length', { writable: false }); | ||
|
|
||
| // Wrap in try catch for older browsers that don't support setting length of |
There was a problem hiding this comment.
I don't see any reference in the spec about this particular operation (https://tc39.github.io/ecma402/#sec-supportedlocales), maybe we can just remove it all together. Not sure why was it added.
There was a problem hiding this comment.
try running the unit tests with this operation removed. If everything passes, we can remove it. Otherwise I will be fine with the try/catch.
There was a problem hiding this comment.
Fixed the lint errors and ran the tests. All of the unit tests passed but I did get a failure when trying to run Sauce Labs.
throw new Error('Could not start Sauce Tunnel');
More than happy to update my MR or implement an alternative.
There was a problem hiding this comment.
Yes, that's fine, that's saucelab script. Update the PR and I will merge it!
There was a problem hiding this comment.
@caridy updated.
Ran into this error on FF 20 so I figured we could wrap it in a
try catchso allow for more tolerant browsers to take advantage of it while keeping those that do not from failing completely. If there is another preferred strategy for handling it, I'm more than happy to implement it. Thanks!