Wrong 3P frame.html loaded when testing local compiled binary

[lannka]

All 3P frames are loading //ampproject.net/dist.3p/1513120604167/frame.html.
This might be due to this change #12152 , which sets config.localDev=true,
hence triggered the following logic in 3p-frame.js:

function getAdsLocalhost(win) {
  if (urls.localDev) {
    return `//${urls.thirdPartyFrameHost}`;
  }
  return 'http://ads.localhost:'
      + (win.location.port || win.parent.location.port);
}

To reproduce,

gulp dist --fortesting
gulp serve --fortesting --compiled

then load page http://localhost:8000/examples/ads.amp.html

[rsimha]

@lannka Setting localDev=true with --fortesting is intentional. The fact that we weren't doing so in the past is a bug that affected use cases like a4a. See #12054.

Can you provide some context for why you expect localDev to be false when you are building the runtime in --fortesting mode? Is this something that can be fixed in getAdsLocalhost?

[lannka]

I do expect localDev to be true (originally proposed here.

And I agree the fix should be somewhere getAdsLocalhost.

What is unclear to me is the current relation between getMode().LocalDev vs AMP_CONFIG.localDev.

If we can consolidate these 2 variables, then we can probably get rid of this getAdsLocalhost function.

[rsimha]

I'm adding @bradfrizzell for comment.

Yes, this bug was exposed by #12152. The issue is captured under #12054 (comment), and is assigned to @bradfrizzell for a fix.

@lannka, some observations:

• There is no getMode().LocalDev. I assume you were talking about getMode().isLocalDev.
• The value of getMode().isLocalDev is written to AMP_CONFIG.localDev, and is refererred to below as "local dev mode". I don't think any more consolidation is required (or possible). @lannka, let me know if you see something I'm missing.
• Local dev mode is enabled by default for gulp build and gulp dist --fortesting.
• After running gulp serve (after gulp build) or gulp serve --compiled (after gulp dist --fortesting), you can check the values of AMP_CONFIG.localDev from the dev console.

At this point, a fix needs to be made by whoever owns the 3rd party ad frame code.

[rsimha]

I have a fix out for review at #12458

[lannka]

There is no getMode().LocalDev. I assume you were talking about getMode().isLocalDev

I'm looking at https://github.com/ampproject/amphtml/blob/master/src/mode.js#L104

The value of getMode().isLocalDev is written to AMP_CONFIG.localDev

This is great. Can you share a code pointer where this is done?

I have a fix out for review at #12458

The fix makes sense.

[rsimha]

Right, I'm looking at the same place. There is no LocalDev (upper case L). See line 79 for the assignment to isLocalDev.

[rsimha]

And see https://github.com/ampproject/amphtml/blob/master/build-system/tasks/prepend-global/index.js#L164 for where we enable localDev in AMP_CONFIG.

[rsimha]

I agree that the logic is complicated. This is because the config is written during build time, and returned at runtime. I wonder how they can be consolidated without a file read.