Skip to content

Conversation

@brendarearden
Copy link

@brendarearden brendarearden commented Oct 4, 2023

Motivation and Context

#2385

Description

Previously I had introduced this change which didn't allow for all paths to be processed appropriately.

      if (external && $Ref.is$Ref(obj)) {
        /* Correct the reference in the external document so we can resolve it */
        let withoutHash = url.stripHash(path);
        if (url.isFileSystemPath(withoutHash)) {
          /* remove file:// from path */
          withoutHash = url.toFileSystemPath(withoutHash);
        }
        obj.$ref = withoutHash + obj.$ref;
      }

I had also attempted to only process browser file paths which still left windows paths broken:

        const isFileUrl = withoutHash.substr(0, 7).toLowerCase() === "file://";
        if (isFileUrl) {
          // Strip-off the protocol
          withoutHash = withoutHash.substr(7);
        }

and additionally added the browser path only change to the test helper #51

These were too narrow in their approach to ensure ALL paths were resolved correctly.

I am now always calling toFileSystemPath with no additional checks to ensure they are processed appropriately.
I also rolled back the changes to the test helper so it also uses the toFileSystemPath

How Has This Been Tested?

Locally on macos
Locally on windows 11 VM

Screenshot(s)/recordings(s)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

@brendarearden brendarearden requested review from a team and chohmann and removed request for a team October 4, 2023 22:06
@brendarearden brendarearden force-pushed the fix/2385-external-ref-resolution branch from dacfd8c to 15d4514 Compare October 4, 2023 22:09
@brendarearden brendarearden requested review from EdVinyard and removed request for chohmann October 4, 2023 22:11
let withoutHash = url.stripHash(path);
withoutHash = url.removeFileProtocol(withoutHash);
obj.$ref = withoutHash + obj.$ref;
obj.$ref = url.toFileSystemPath(withoutHash) + obj.$ref;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @matthewmurphy and the other speed-runners say, "First try!" 😆

@mmarti21 mmarti21 merged commit 8d19b3b into master Oct 5, 2023
@mmarti21 mmarti21 deleted the fix/2385-external-ref-resolution branch October 5, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants