Skip to content
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

put back localhost check for localdev #11783

Merged
merged 1 commit into from Oct 23, 2017

Conversation

erwinmombay
Copy link
Member

@rsimha-amp do you remember why you removed that line?

@rsimha
Copy link
Contributor

rsimha commented Oct 23, 2017

This was part of what @lannka suggested in the original issue. Let me dig in some more.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Turns out the localhost check doesn't always work, and so, @lannka wanted it gone in lieu of always applying the prod config to local builds. See #8008 (comment)

In any case, since others are complaining that ads don't load without the localhost check, I'm going to approve this and take a closer look into what else we can do.

@erwinmombay erwinmombay merged commit c1af578 into ampproject:master Oct 23, 2017
@rsimha
Copy link
Contributor

rsimha commented Oct 23, 2017

For posterity, see #11588 for the PR that removed the localhost check

@lannka
Copy link
Contributor

lannka commented Jan 3, 2018

@erwinmombay @rsimha-amp do you recall what was the test pages got broken?

@rsimha
Copy link
Contributor

rsimha commented Jan 3, 2018

@lannka the ads in http://localhost:8000/examples/ads.amp.html were 404ing without the localhost check

@lannka
Copy link
Contributor

lannka commented Jan 3, 2018

All ads broke because removing the localhost check makes isLocalDev false. hence all ads start to load 3p bootstrap frame from production with a wrong RTV path.

Let's discuss the solution on this thread: #12054

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants