perf(gatsby-source-filesystem): dont JSON parse/stringify the node

[pvdz]

This seems to make a few seconds difference at scale. It's also highly unnecessary. And perhaps it has a butterfly effect at scale wrt the GC.

Still benchmarking to confirm the delta. Roughly speaking, the 64k gabe-fs-text seems to save ~2s for this change (which is pretty minor in the grand scheme of things).

[pvdz]

gabe-fs-text wins about 1.5s at 64k files with this.

[vladar]

Maybe we can have a snapshot-based test for a dummy file and directory so that we could see actual before/after structures?

Somewhere here:

gatsby/packages/gatsby-source-filesystem/src/__tests__/create-file-node.js

Line 7 in 18c182a

describe(`create-file-node`, () => {

Edit: looks like the only existing test in __tests__/create-file-node has no assertions 🤔

[pvdz]

I've added a snapshot test for this. The snapshot was the same on master.

[pvdz]

There are some windows pathing problems so I've tried to work around it. Hope this holds. Before state:

[vladar]

@pvdz You can do this snapshot this way:

expect(node).toMatchInlineSnapshot({
  accessTime: expect.any(String),
  atime: expect.any(String),
  atimeMs: expect.any(Number),
  absolutePath: expect.any(String),
  dir: expect.any(String),
  root: expect.any(String),
})

Then re-run and it will generate the universal snapshot (it will ignore actual values but at least all the keys will be there).

The sad part is that we use jest-serializer-path to normalize paths in snapshots but it doesn't work here because we slash paths in createFileNode and so on windows jest-serializer-path doesn't understand that those are paths 😬

As for dates - they seem to be working in CI but not locally for some reason (in CI dates have a timestamp 0 but locally they fill it with the current date value for me)

[pvdz]

I'm not sure I like that any better than my current solution. I mean I'm also not very happy with having to create a fake file and all that, but the alternative to that is mock out more of fs which I don't want to do either.

At some point a test is too tightly bound to the implementation rather than the feature and it becomes a burden to maintain. I think mocking out fs.stat is a fair tradeof. I could even omit half the props on it since the test (due to its mocks) doesn't really care about that. Just that these Date props are serialized.

Anyways, I think it's fine like this. The test passes.

[vladar]

Nicely done, thanks! 👍

[wardpeet]

👍