Remove all the hard coded "http://localhost:8000" in tests and examples #3522
Conversation
…pages. Replace with relative URLs to be compatible with HTTPS server.
lannka
changed the title
| @@ -66,8 +66,8 @@ | |||
| <amp-user-notification | |||
| layout=nodisplay | |||
| id="amp-user-notification1" | |||
| data-show-if-href="http://localhost:8000/api/show?timestamp=TIMESTAMP" | |||
There was a problem hiding this comment.
so not sure about doing relative paths on these because as examples they were supposed to convey doing requests to a different domain (granted we didn't even do that, we should have maybe spun up a new local server with a different port)
/cc @cramforce
There was a problem hiding this comment.
I don't know if "localhost" is really conveying the cross domain ability. One other compromise is to strip off the schema part and keep the domain name: "//localhost:8000". But that will still break if a different port is used.
Launching another local server might be too much for the purpose of demonstration. I personally prefer the relative path solution. On the other hand, we could always improve our documentation to cover those details.
There was a problem hiding this comment.
Does this actually work? I thought we might mandate https to be present.
There isn't really a great way to do this, I'm afraid, short of producing other example files (like we we do for .min.) We could do .https..
There was a problem hiding this comment.
It worked via localhost, but I did get an error when accessing via http://dinghongfei.mtv.corp.google.com.
source must start with "https://" or "//" or be relative and served from either https or from localhost. Invalid value: /api/show?timestamp=TIMESTAMP _reported_
Why don't we allow relative URLs over HTTP? I don't see it mentioned in the amp-user-notification doc. Is it something general we tend to disallow in the whole AMP project? If so, could you point me to a reference?
There was a problem hiding this comment.
We don't allow it, because we want people to understand that while they can deploy the document on HTTP, these resources must be available on HTTPS.
There was a problem hiding this comment.
If we want to really force resources over HTTPS, then we should not allow "//" as well. Otherwise document on HTTP would still load non-HTTPS resources ?
There was a problem hiding this comment.
Yeah, but we can't change it anymore :)
There was a problem hiding this comment.
I see.
Nevertheless, relative URLs on localhost or over 'https' are exempted from this check. So this PR works.
Regarding to your suggestion about having 2 examples, one with https://localhost:8000 and one with http://localhost:8000, it actually doesn't really demonstrate the requirements either. The requirement is basically saying: 1) use https. 2) for localhost, do whatever. This PR demostrate 2), but 1) is really hard to demonstrate. Because we need a non-localhost https server running.
Not sure if it is worth to start such a server on heroku?
There was a problem hiding this comment.
hmm that might be a good idea, these examples were built to be copy paste-able and they aren't with localhost on the endpoints. we can just deploy this repo and it should just work (i think)
|
LGTM. we'll eventually switch it to the server endpoints when we have it up. related: #3560 |
Replace with relative URLs to be compatible with HTTPS server.
Related to #3499