Performances problems fixes on big files with a lot of references on same big object

[paztis]

fixes #176.

add processedObject list to avoid recursion in already processed tree parts
add dereferencedCache to avoid to re-treat object already dereferenced

[paztis]

I was working on a file that takes 10s to be processed (11 millions of passes in the crawl function)
I add caches to avoid this, and now we only pas 35000 times inside

[marbemac]

Very nice - just curious, could the bundle method benefit from a similar caching mechanism?

[paztis]

I don't look at the bundle usage. I only use this one, but potentially yes.

[paztis]

the same dev will be to redo

[paztis]

@P0lip any updates on the review ?

[marbemac]

He's on vacation until early November, we'll have time to check it out then!

[P0lip]

Hey @paztis.
As Marc stated, I'm off until the end of this week.
I'll try to get back to you on Monday, next week (02 Nov)

[Eli-Black-Work]

Hey @paztis.
As Marc stated, I'm off until the end of this week.
I'll try to get back to you on Monday, next week (02 Nov)

Enjoy your vacation 🙂

[P0lip]

Would be great to add some tests for the properties merging (line 125).
Looking pretty solid otherwise

[P0lip]

This might screw the order of properties and override some of them.
I'd suggest we bail out to regular scenario, same as we do in case of storing the cache at line 176.
Alternatively, we could maintain 'before$RefKeys' and after$RefKeys and merge them in that order.

[paztis]

yes it will set extraKeys after basic ones
Is this order important ?

I do it to support some unit tests where an object has a description, then a reference of it overrides the description.
The cache key is the same for both (reference is unique). But after dewreference, ths description in both versions is not the same.
You've got specific unit tests for this.

[P0lip]

Is this order important ?

Yeah it might matter at times if you have the property with the same key - it'll get overriden, won't it?
IMHO the current implementation may have a minor bug here.
I went through tests and we don't seem to have anything that'd validate the scenario I mentioned.

The below scenario fails

  it("should preserve the order of properties", async () => {
    let parser = new $RefParser();
    const schema = await parser.dereference(
      // This file doesn't actually need to exist. But its path will be used to resolve external $refs
      path.abs("path/that/does/not/exist.yaml"),
      // This schema object does not contain any external $refs
      {
        foo: {
          minLength: 1,
        },
        bar: {
          $ref: "#/foo",
        },
        baz: {
          $ref: "#/foo",
          minLength: 4,
        },
      },
      // An options object MUST be passed (even if it's empty)
      {});

    expect(schema).to.deep.equal({
      foo: {
        minLength: 1
      },
      bar: {
        minLength: 1
      },
      baz: {
        minLength: 4
      },
    });
  });

An improved implementation could look as follows

  if (refKeys.length > 1) {
      const value = {};
      for (let key of refKeys) {
        if (key === "$ref") {
          Object.assign(value, cache.value);
        }
        else {
          value[key] = $ref[key];
        }
      }

      return {
        circular: cache.circular,
        value,
      };
    }

This is questionable, however, since, if I am not mistaken, until draft 2019-09 you couldn't place $refs to any other properties, but as of 2019-09 you can and IIRC they are merged similarly to YAML merge keys.
I took at look at the spec, but couldn't really find any information regarding the order of merging.
Perhaps @philsturgeon has a clue on this one?

[steverice]

FWIW this resolves the same issue that I have with an OpenAPI schema containing many (hundreds) of refs.
@P0lip @philsturgeon is this on track to be merged soon?

[P0lip]

@steverice there is one (minor) suggestion left to be addressed.
I think we can merge it as is and I could apply the comment later on. It's non-blocking.

[tahpot]

Thanks to all for the work on this. Fantastic!

[Eli-Black-Work]

@P0lip Can we get a new release on NPM? 🙂 (Beta would be fine)