⚡ Performance improvements when using spread in reduce

[KenAJoh]

Description

As a test im using Biome to do some linting and slowly fixing some of the errors found.

This PR is based around the rule: https://biomejs.dev/linter/rules/no-accumulating-spread/

In real applications these changes will have marginal effect for our use since we rarely handle large arrays. The only exception i see is combobox/FilteredOptions/filteredOptionsContext.tsx where one in theory could end up having arrays in the thousand or more. (but again the performance-issues from just having 10k+ dom nodes would probably be the real problem long before this)

Made a small test-script to compare performance

const { performance } = require("perf_hooks");

function createArray(length) {
  return new Array(length).fill(0).map((_, i) => i + 1);
}

function testPerformance(array, useSpread) {
  const start = performance.now();

  array.reduce((acc, val) => {
    if (useSpread) {
      return [...acc, val];
    } else {
      acc.push(val);
      return acc;
    }
  }, []);

  const end = performance.now();

  return (end - start).toFixed(3);
}

function main() {
  const lengths = [10, 100, 1000, 10000, 100000];

  for (const length of lengths) {
    const array = createArray(length);

    const timeWithSpread = testPerformance(array, true);
    console.log(`Time with spread for length ${length}: ${timeWithSpread}ms`);

    const timeWithoutSpread = testPerformance(array, false);
    console.log(
      `Time without spread for length ${length}: ${timeWithoutSpread}ms`,
    );
  }
}

main();

[changeset-bot]

🦋 Changeset detected

Latest commit: e5ca8c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@navikt/ds-react	Patch
@navikt/ds-css	Patch
@navikt/ds-tokens	Patch
@navikt/ds-tailwind	Patch
@navikt/aksel-icons	Patch
@navikt/aksel	Patch
@navikt/aksel-stylelint	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

Storybook demo

d7147cb7f | 81 komponenter | 182 stories

[HalvorHaugan]

For some weird reason, name becomes any now 🙃

I found that one way of fixing it is to add type to Object.entries. I also think we can remove the nested map():

  const globalRefs = Object.entries<
    { name: string; value: string | number; description?: string }[]
  >(docs)
    .filter(([key]) => key.startsWith("global-"))
    .flatMap(([, value]) => value);

This is a bit ugly though, so you probably want to extract the type.

It might be that it's actually cleaner to use map() and flat() separately in this case, b.c. then we don't get that type issue:

  const globalRefs = Object.entries(docs)
    .filter(([key]) => key.startsWith("global-"))
    .map(([, value]) => value)
    .flat();

But maybe you can find a better way to fix that type bug...

[KenAJoh]

Cound not find a better way to preserve types, but biome complains when not using flatmap, so had to "hack" it by adding the flat() in the return

const globalRefs = Object.entries(docs)
    .filter(([key]) => key.startsWith("global-"))
    .map(([, value]) => value);

  return (
    globalRefs
      .flat()
      .find(
        ({ value, name }) =>
          semanticValue === value && notBlacklistedName(name),
      ) ?? null
  );

[HalvorHaugan]

Seems like you forgot to push the changes, so I will do it.

[HalvorHaugan]

Should we have a changeset for Combobox? 🤔

[HalvorHaugan]

@JulianNymark @it-vegard @larseirikhansen What do you think?

[KenAJoh]

If we follow the semver strategy stictly this would be a patch, but for such small internal changes i often feel it could just hitch a ride on the next update we push. But if we want as a team we could force all changes to be patches in these scenarios. What does everyone think?

[JulianNymark]

I also think small refactoring should be fine to do without needing a changeset, since the changeset is mostly for external eyes, and I guess they would mostly care about features and bugfixes, (maybe performance stuff), but plain refactoring gives users nothing right? (if they inspect our code it might be more readable).

[JulianNymark]

@HalvorHaugan you mean add a changeset to combobox for this change right?

In this case it's a only a refactoring right? (pulling out initialMap from within the reduce())

[HalvorHaugan]

That was what I meant, yes. Mainly bc. it's a performance improvement. If it was only a plain refactor that no-one would notice, I would not have a changeset for it.

[JulianNymark]

yeah, I'm for adding it! ⚡ (I didn't realize it was perf in that instance, but that's the whole PR's purpose 😳 )