Page MenuHomePhabricator
Differential D8728

Switch back to janky array copying
ClosedPublic
Actions

Authored by sophiebits on Apr 9 2014, 12:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 15, 2:36 AM
Unknown Object (File)
Fri, Dec 13, 8:53 PM
Unknown Object (File)
Mon, Dec 9, 2:21 PM
Unknown Object (File)
Sat, Dec 7, 9:12 PM
Unknown Object (File)
Wed, Dec 4, 1:26 AM
Unknown Object (File)
Sat, Nov 30, 10:38 AM
Unknown Object (File)
Nov 27 2024, 8:52 AM
Unknown Object (File)
Nov 27 2024, 7:04 AM
View All Files
Subscribers
chad
cpojer
epriestley
Korvin
tomo

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
Restricted Diffusion Commit
rP597c6c07f712: Switch back to janky array copying
Summary

Bad news @cpojer @tomo. IE8 doesn't like you.

Test Plan

Load a diff in IE8; see changes and don't get JS errors.

Diff Detail

Repository
rP Phabricator
Branch
jxa
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

sophiebits updated this revision to Diff 20696.Apr 9 2014, 12:57 AM
sophiebits retitled this revision from to Switch back to janky array copying.
sophiebits updated this object.
sophiebits edited the test plan for this revision. (Show Details)
sophiebits added a reviewer: epriestley.
sophiebits added subscribers: cpojer, tomo.
Herald added a reviewer: Blessed Reviewers.Apr 9 2014, 12:57 AM
Herald added subscribers: Korvin, epriestley.
epriestley requested changes to this revision.Apr 9 2014, 1:00 AM
epriestley edited edge metadata.

hahahahaha

oh well, we can do this again in 6 years once IE8 is deprecated

webroot/rsrc/externals/javelin/core/util.js
57–61

Let's just do this one unconditionally? I'd rather have simpler code than the presumably-trivial performance gains of invoking slice.

This revision now requires changes to proceed.Apr 9 2014, 1:00 AM
sophiebits added a comment.Apr 9 2014, 1:00 AM

(Facebook's toArray corroborates this story: https://github.com/facebook/react/blob/master/src/vendor/core/toArray.js says "IE < 9 does not support Array#slice on collections objects".)

webroot/rsrc/externals/javelin/core/util.js
57–61

Sure.

epriestley added a comment.Apr 9 2014, 1:00 AM

Oh, one thought: if we force edge compatibility mode, maybe that fixes this?

sophiebits updated this revision to Diff 20697.Apr 9 2014, 1:03 AM
sophiebits edited edge metadata.

Consistent clowny copying

sophiebits updated this revision to Diff 20698.Apr 9 2014, 1:05 AM
sophiebits edited edge metadata.

Make it actually work and test it

cpojer added a comment.Apr 9 2014, 1:07 AM

booooooo

epriestley accepted this revision.Apr 9 2014, 1:07 AM
epriestley edited edge metadata.

bwahaha

This revision is now accepted and ready to land.Apr 9 2014, 1:07 AM
epriestley closed this revision.Apr 9 2014, 1:08 AM
epriestley updated this revision to Diff 20699.

Closed by commit rP597c6c07f712 (authored by @spicyj, committed by @epriestley).

chad added a subscriber: chad.Apr 9 2014, 1:08 AM

Are these actual ie8 browsers? (We should force edge anyways)

sophiebits added a comment.Apr 9 2014, 1:09 AM

I tested it with a real, live IE8 browser in "IE8 Standards" mode.

Revision Contents
Changeset List

PathSize
resources/
celerity/
6 lines
webroot/
rsrc/
externals/
javelin/
core/
12 lines

Diff 20696

View Options

resources/celerity/map.php

Loading...
View Options

webroot/rsrc/externals/javelin/core/util.js

Loading...
Phabricator · Built by Phacility