Fix: Allow absolute URLs as paths, e.g. for load()#2056
Open
tchebb wants to merge 1 commit intoprotobufjs:masterfrom
Open
Fix: Allow absolute URLs as paths, e.g. for load()#2056tchebb wants to merge 1 commit intoprotobufjs:masterfrom
tchebb wants to merge 1 commit intoprotobufjs:masterfrom
Conversation
tchebb
commented
Apr 4, 2025
| * @returns {string|null} the prefix, or null if path is relative | ||
| */ | ||
| export function absolutePrefix(path: string): (string|null); | ||
|
|
Author
There was a problem hiding this comment.
Is there a script to update these index.d.ts files? I saw that npm run build:types updates the one at the root, but I didn't see anything similar for these, so I just made the change manually.
tchebb
force-pushed
the
support-absolute-urls
branch
from
862eb9f to
578768c
Compare
When using protobuf.js in a browser, it's useful to be able to load proto files from other origins. But currently, path.normalize() mangles URLs by removing the second slash after the scheme (e.g. "https://example.com/" becomes "https:/example.com"). Fix that issue, and better integrate the existing UNC support, by introducing a new path.absolutePrefix() function that checks for all the various prefixes that indicate a path is absolute and returns the one it finds (if any). By including both slashes in the return value for URLs, we exclude them from the normalization logic. Note that this almost certainly still isn't perfect, as URLs, UNC paths, and even Windows drive letters are all quite complex. But I don't have the time or project context to make a major change here.
tchebb
force-pushed
the
support-absolute-urls
branch
from
578768c to
f21d293
Compare
Author
|
Note that this fixes the same problem that #1820 does. I didn't notice that PR before making this one, although this one has the added benefit of supporting additional URL schemes and simplifying the UNC code too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When using protobuf.js in a browser, it's useful to be able to load proto files from other origins. But currently,
path.normalize()mangles URLs by removing the second slash after the scheme (e.g.https://example.com/file.jsbecomeshttps:/example.com/file.js).Fix that issue, and better integrate the existing UNC support, by introducing a new
path.absolutePrefix()function that checks for all the various prefixes that indicate a path is absolute and returns the one it finds (if any). By including both slashes in the return value for URLs, we exclude them from the normalization logic.Note that this almost certainly still isn't perfect, as URLs, UNC paths, and even Windows drive letters are all quite complex. But I don't have the time or project context to make a major change here.