-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix: Allow absolute URLs as paths, e.g. for load() #2056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what your workflow is for updates inside lib/. I didn't bump the version in lib/path/package.json, but I assume that'll be needed for protobuf.js to see these changes. Let me know if I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
578768c to
f21d293
Compare
tchebb
commented
Apr 12, 2025
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.
Uh oh!
There was an error while loading. Please reload this page.
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.