eleventy icon indicating copy to clipboard operation
eleventy copied to clipboard

Unused code in Merge.js

Open Snapstromegon opened this issue 3 years ago • 1 comments

I believe that the lines 14-18 are from an old version, since they are not hit by UT coverage and probably don't even work as the author expected.

https://github.com/11ty/eleventy/blob/fc6fc96707ad9ca062622ea11e0c092b666372a9/src/Util/Merge.js#L11-L44

I came across this, since I'm currently working on raising UT coverage and I wasn't able to hit line 17 at first.

This is a case which hits said line, but probably isn't what you'd expect.

t.deepEqual(
    Merge(
      {
        a: {
          b: {
            c: [1],
          },
        },
      },
      {
        a: {
          "override:override:b": {
            c: [2],
          },
        },
      }
    ),
    {
      a: {
        b: {
          c: [1],
        },
        "override:b": {
          c: [2],
        },
      },
    }
  );

This happens, because in line 30 the "override:"-prefix already gets removed, so the condition in line 15 can only be true, if the parentKey is prefixed twice.

Removing line 14-18 keeps all unittests passing, but I'd like to have a second opinion before I create a PR with this.

Snapstromegon avatar Jun 30 '22 16:06 Snapstromegon

Also that his test passes, is a little unexpected to me:

  t.deepEqual(
    Merge(
      {
        a: {
          "override:b": {
            c: [1],
          },
        },
      },
      {
        a: {
          "override:b": {
            c: [2],
          },
        },
      }
    ),
    {
      a: {
        b: {
          c: [1,2],
        },
        "override:b": {
          c: [1,2],
        },
      },
    }
  );```

Snapstromegon avatar Jun 30 '22 16:06 Snapstromegon

Filed your last comment as a new issue https://github.com/11ty/eleventy/issues/2684

zachleat avatar Dec 08 '22 21:12 zachleat

Removed, shipping with 2.0.0-canary.19!

zachleat avatar Dec 08 '22 21:12 zachleat

I came across this while debugging/running some diagnostics with our implementation of Eleventy. We're using Eleventy v1.0.2 at the moment on a rather large project, and running into heap issues with Node.js. We've put a bandaid on it by increasing the old memory size of the heap for some time.

Now that I've started to run and analyze the heap profiler in Node.js (node --heap-prof), it appears that this getMergedItem function was responsible for ~ 1.4 GiB on the heap.

Will need to do more digging. Any insight into how getMergedItem is used that might cause it to be a candidate for high memory consumption?

zbroniszewski avatar Aug 10 '23 01:08 zbroniszewski