✨ Add User Agent Client Hint Params to Google ad request params

[zombifier]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[zombifier]

Hi all,
reviving this since server-side support should have landed. PTAL.

[calebcordry]

Also not sure if you are aware but we allow async/await in our tests if you want to use it.

[calebcordry]

@rcebulko I think the bundle size check here might be bad? Any idea what's going on?

[rcebulko]

@rcebulko I think the bundle size check here might be bad? Any idea what's going on?

IIRC there was a big a while back, but this is an old PR so it probably doesn't have the fix. Rebasing should fix hopefully

[calebcordry]

Thanks @rcebulko Khoi can you try rebasing?

[zombifier]

The ads/google/a4a/utils.js must not depend on src/core/data-structures/promise.js error persists. Strange. Is it possible that the check's indeed triggered by usages of Promise in the PR and that the rest of the code is older than the check so they were "grandfathered" in?

[calebcordry]

@rsimha Could you help us with the dep-check on this one? It seems to think we introduced a new dependency ads/google/a4a/utils.js -> src/core/data-structures/promise.js but all that has been added is a Promise.resolve(). I can repro by adding a line anywhere in the ads/google/a4a/utils.js file const foo = Promise.resolve()

[rcebulko]

@rsimha Could you help us with the dep-check on this one? It seems to think we introduced a new dependency ads/google/a4a/utils.js -> src/core/data-structures/promise.js but all that has been added is a Promise.resolve(). I can repro by adding a line anywhere in the ads/google/a4a/utils.js file const foo = Promise.resolve()

We have a transformer that rewrites all empty Promise.resolve() calls to use the same resolvedPromise object defined in that file, rather than re-creating a fresh Promise object and resolving it each time (much slower/more memory taken). You can add the dependency for now, and maybe we should allowlist that dependency more generally?

[calebcordry]

thanks again @rcebulko! Makes sense but agree we should do something if we can to help future users, it's pretty unclear what is happening.

[zombifier]

Added the dependency. Thanks all for the help.

[rcebulko]

@rsimha Do we have a mechanism for allowlisting code in dep-check? Really anything from #core is safe to be imported from anywhere.

[rsimha]

@rsimha Do we have a mechanism for allowlisting code in dep-check? Really anything from #core is safe to be imported from anywhere.

Yes, there is an allowlist mechanism. I'd imagine it's possible to allow all imports from src/core by changing this glob in dep-check-config.js to include everything in src/ except src/core.

amphtml/build-system/test-configs/dep-check-config.js

Line 126 in 7741dcc

mustNotDependOn: 'src/**/*.js',

It appears that matchBadDeps() in dep-check.js expands globs and arrays of globs, but it might take some trial and error to get them to do what you'd like.

/cc @erwinmombay @samouri @jridgewell who should know more about these rules than I do.

[samouri]

I agree what we should allow core imports everywhere

[rcebulko]

The glob we would want is src{,/!(core)/**}/*.js. I can throw together a PR for this