-
Notifications
You must be signed in to change notification settings - Fork 312
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
Allows query params in precomputed chunks #322
base: master
Are you sure you want to change the base?
Allows query params in precomputed chunks #322
Conversation
This patch allows the Content-Type header to have any case and also allows charset specification, like so: Cotent-type: text/html; charset=UTF-8 which wasn't allowed before
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
let match = url.match(urlProtocolPattern); | ||
if (match === null) { | ||
throw new Error(`Invalid URL: ${JSON.stringify(url)}`); | ||
export class ParsedUrl{ |
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.
Can we use the existing URL class already provided by browsers, or is it unsuitable for some reason?
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 figured eventually ParsedUrl
(or whatever other name you might want to give it) could be used to handle URLs in more places throughout the code where URL manipulation is required. The native implementation would be insufficient because it silently breaks on funny protocols:
new URL("gs://example.com/some/path")
// produces URL { (...) host: "", hostname: "", port: "", pathname: "//example.com/some/path"}
// note the empty hostname, which was interpreted as being part of the pathname
So even if we extended URL
, we'd still need to do the parsing ourselves.
Still, if we split handling the special Neuroglancer URLs (e.g. precomputed://http://...
, gs://
) from the expanded, canonical URLs (i.e.: https?:
), then extending the native URL
would be enough. But I think unifying all URL handling under a single class would be simpler to follow.
37bd2ef
to
4b93929
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
ParsedUrl
class to streamline handling URLsParsedUrl
everywhere the PrecomputedChunks datasource composes URLs, concatenating paths and preserving query stringsFixes #316
Needs #320
TODO: