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

3p-frame: Prefer #core/mode to getMode #35669

Merged
merged 6 commits into from
Aug 13, 2021
Merged

Conversation

caroqliu
Copy link
Contributor

This change enables 3p iframe Bento components to reference the mode without having to be on an AMP document.

src/3p-frame.js Outdated Show resolved Hide resolved
function mockMode(mode) {
env.sandbox.stub(window.parent, '__AMP_MODE').value(mode);
function mockMode(options) {
env.sandbox.stub(window.parent, '__AMP_MODE').value(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the one call where removing the line breaks the test case:

mockMode({
localDev: true,
development: false,
test: false,
version: '$internalRuntimeVersion$',
});

It looks like this is specifically because the mode changes when not mocked via window:

'mode': getModeObject(),

I propose we leave the stub for now, and in a separate PR, see about updating getModeObject to reference #core/mode over getMode(win):

export function getModeObject(opt_win) {
return {
localDev: getMode(opt_win).localDev,
development: getMode(opt_win).development,
esm: IS_ESM,
test: getMode(opt_win).test,
rtvVersion: getMode(opt_win).rtvVersion,
};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

test/unit/3p/test-3p-frame.js Show resolved Hide resolved
Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

It looks like this is not being DCE'd correctly; the component bundle sizes are spiking. Taking a look now

Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

I had seen ~150B increase in some bundles, but I locally built this PR and its merge base and compared the files directly to find no size diff. Looks like an artifact of CI using a different merge base or something similar. Re-approving

@caroqliu
Copy link
Contributor Author

I had seen ~150B increase in some bundles, but I locally built this PR and its merge base and compared the files directly to find no size diff. Looks like an artifact of CI using a different merge base or something similar. Re-approving

Thanks for looking into this!

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