Closed Bug 1260589 Opened 4 years ago Closed 4 years ago

Add the ThreadSafeDevToolsUtils.flatten utility

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 1 obsolete file)

The `flatten` function takes an array of arrays and flattens them to a single
array, removing one level of nesting. It does not recursively flatten multiple
levels of nesting.
Assignee: nobody → nfitzgerald
Blocks: 1249788
Status: NEW → ASSIGNED
Comment on attachment 8736069 [details] [diff] [review]
Add the ThreadSafeDevToolsUtils.flatten utility

Jordan is on PTO, so Jim gets another review!
Attachment #8736069 - Flags: review?(jsantell) → review?(jimb)
Comment on attachment 8736069 [details] [diff] [review]
Add the ThreadSafeDevToolsUtils.flatten utility

Review of attachment 8736069 [details] [diff] [review]:
-----------------------------------------------------------------

If the identities I suggest here are actually right, then perhaps it's best to just use the primitive that JS readers would be familiar with directly, instead of introducing new names for them.

::: devtools/shared/ThreadSafeDevToolsUtils.js
@@ +248,5 @@
> +  const length = inList.length;
> +  for (let i = 0; i < length; i++) {
> +    outList.push(inList[i]);
> +  }
> +  return outList;

If this is required to mutate outList, then I think this is equivalent to outList.push.apply(inList). If only its return value matters and its arguments are consumed, then this is equivalent to outList.concat(inList). But I am not at all comfortable with the JS array operations, so check my claims.

@@ +259,5 @@
> + * @param {Array<Array<Any>>} lists
> + * @return {Array<Any>}
> + */
> +exports.flatten = function(lists) {
> +  return lists.reduce(flattenReducer, []);

I think this is just [].concat.apply(lists).
Attachment #8736069 - Flags: review?(jimb) → review+
The `flatten` function takes an array of arrays and flattens them to a single
array, removing one level of nesting. It does not recursively flatten multiple
levels of nesting.
Attachment #8736385 - Flags: review+
Attachment #8736069 - Attachment is obsolete: true
We should probably just use lodash (or the subset that isn't covered by the ES spec) at some point
We should probably just use left-pad...
</snark>

We should probably replace this with Array.prototype.flatten once SM implements it.
https://hg.mozilla.org/mozilla-central/rev/1ac6445b0025
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.