feat(ads-sdk): attempt to integrate ads-sdk

[mashalifshin]

Goals

This is an early attempt to integrate the ads sdk into Pocket and shake out any bugs or issues as early as possible. It is currently WIP, the goal here is to iterate and troubleshoot, and this branch may be discarded in favor of a clean implementation once we finish shaking out the bugs.

To Done:

• Figure out why I had trouble with the git+ssh package.json urls, and/or start the process to push to mozilla's private npm. I ended up just including the sdk repo's dist folder into node_modules until we can vend via npm
• Figure out why there are errors referencing dist/core. Ended up removing the dist in the import paths and that resolved this issue for this build (though we'll want to check this again with a proper vend via npm install instead of manually adding the dep)
• Error after resolving the above, it's a runtime error now. The page loads and is viewable for a moment and then fails with Unhandled Runtime Error TypeError: null is not an object (evaluating 'resolveDispatcher().useState'). This is fixed now, was due to a bundling bug where client-fetch was being required instead of being bundled.
• New error with integrating latest ads-sdk main at e48e23f. ReferenceError: navigator is not defined. I think this has to do with the new logger code, and perhaps server side rendering when it shouldn't be?

To Do Pocket-side:

• Seeing the "jumping" behavior (aka Freestar style 😭) where when I first load the page, the space for the moz ad is not reserved, so when the moz ad starts loading, the content jumps down abruptly to lower down on the page. I'll post a video.

Justin, Luc, and I took a closer look and we think this is the reflow happening based on the allowAds decision which decides whether to show the component or not based on if the user has premium. Looking closely, seems like we have improved on the Freestar behavior as much as we can on our end (when the component loads, it keeps the space it needs without jumping around), so if we want to go further we could perhaps refactor some surrounding Pocket code? Will be curious to see what Joel thinks.

• Showing a medium rectangle ad format above and below the fold for mobile. I took a crack at it, but it doesn't request different placements when I use browser tools to emulate mobile viewports. Should I be pulling in the useWindowDimensions hook for this?

• Test coverage. I'd love some guidance on where I can add tests for this new stuff.

To Do Ads SDK-side:

• @justindarc Seeing a warning when I first start the dev server: Receivedtruefor a non-boolean attributecrossOrigin. If you want to write it to the DOM, pass a string instead: crossOrigin="true" or crossOrigin={value.toString()}`

• Nit: @luc-lisi the IntersectionObserver not found in the global namespace; Impressions may not be observed or this message is being shown server-side. doesn't seem necessary to show in the server-side dev logs since that's expected in that env. Can we do a check and not show it if we're server side?

Before you Submit

We have some formatting rules in place to keep things consistent and clean:

• Your PR title needs to be formatted as type(context): message
• Your branch name needs to be formatted as type/work-being-done

Valid types are:

• feat: a new feature - this will trigger a minor release
• fix: a fix to an existing feature - this will trigger a patch release
• refactor: refactoring code or UI
• build: focused solely on the build/deployment stages (npm, webpack, ci, etc/)
• docs: documentation updates
• test: updating test suites
• chore: cleanup and automated tasks
• project: only valid in the branch name, represents a jumping off point for a project

[mashalifshin]

This doesn't seem to be working for me, it's giving permissions errors. I think it doesn't know to use my ssh keys for this remote like it does for ads-sdk?

I did try cloning ads-sdk separately to make sure my permissions are correct, but when I try this, or git+ssh url, both fail with

 ERROR  Command failed with exit code 128: git ls-remote git+ssh://[email protected]/mozilla-services/ads-sdk.git HEAD
[email protected]: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

[mashalifshin]

So what I did instead was just copy the entire contents of ads-sdk repo into the clients/web/node_modules which may be the wrong thing to do and the cause of the error?

[collectedmind]

Does this run locally? If it does, perhaps we need to add PocketCI to the permissions on @mozilla-services/ads-sdk?

[mashalifshin]

This only works for local development, yes. We were hoping that using the git urls in package.json would work for a quick boostrap for local dev by leveraging our personal local github ssh keys to pull the sdk repo, but it didn't work for me for this attempt.

That's okay though, regardless we'll need to ultimately vend this to mozilla's NPM registry so that we can pull this dependency in a standard way, and a way that will work for CI and deploy builds.

So if I understand your suggestion correctly, I think we don't want to worry about giving PocketCI permission to the sdk repo. I think we should just work on getting it vended properly. I'll kick off that process today.

[mashalifshin]

We should be finishing up the mozilla npm setup process today, so hopefully we can try a proper vend soon!

[mashalifshin]

Welp, mozilla doesn't pay for the npm private packages, so that means when we push up to npm, we should consider that going public and prepare accordingly. I'm working with my manager Catherine and our Product Marketing lead Bryce to get that squared away ASAP since that blocks "real vending", which in turn blocks this PR from working anywhere but my local machine

[mashalifshin]

@justindarc could you take a look at this next iteration whenever you get a moment? I have brought in the latest main from the ads-sdk repo and fixed the import paths as you suggested yesterday, and I am getting a new error now :

Unhandled Runtime Error
TypeError: null is not an object (evaluating 'resolveDispatcher().useState')

Again, not sure if I've missed something in terms of how to integrate this properly, or if there is an underlying bug in the sdk

[justindarc]

@justindarc could you take a look at this next iteration whenever you get a moment? I have brought in the latest main from the ads-sdk repo and fixed the import paths as you suggested yesterday, and I am getting a new error now :

Unhandled Runtime Error
TypeError: null is not an object (evaluating 'resolveDispatcher().useState')

Again, not sure if I've missed something in terms of how to integrate this properly, or if there is an underlying bug in the sdk

@mashalifshin what version of "react" and "react-dom" is Pocket using?

[mashalifshin]

@mashalifshin what version of "react" and "react-dom" is Pocket using?

Looks like 19.0.0 for both (here)

[mashalifshin]

@justindarc I integrated the latest ads-sdk main at e48e23f. We have a new error, hooray!! 🙌 I think this has to do with the new logger code (tho not gonna message Luc today), and perhaps server side rendering when it shouldn't be?

On the client:

In the server-side logs:

Take a look and lmk if anything obvious jumps out at you, and/or if you'd like to 🍐 again on this like last time

[justindarc]

@mashalifshin I think I see why this is happening. Can you try the code in my PR branch? https://github.com/mozilla-services/ads-sdk/pull/24

[mashalifshin]

@mashalifshin I think I see why this is happening. Can you try the code in my PR branch? mozilla-services/ads-sdk#24

@justindarc @luc-lisi I pulled the latest main (since the branch was merged) and tried it, and it works!!! First MozAd showing on Pocket in local dev!!! 👏 👏 👏

(I do see some potential bugs/warnings/things we might want to to tune, I'll update this PR description with the deets)

[collectedmind]

Seeing the "jumping" behavior (aka Freestar style 😭) where when I first load the page, the space for the moz ad is not reserved, so when the moz ad starts loading, the content jumps down abruptly to lower down on the page. I'll post a video.

Yep! These are static pages, and then we do some injection of things on the client (like user, isPremium, etc.) that let's us show/hide ads etc. Couple of options:

1. we hold this space, then collapse it gracefully if the user is premium (still some thrashing, but we can make it pretty thrashing)
2. we hold off on the render until we have the relevant information (this kinda breaks the whole purpose of static for SEO)
3. We do some css trickery so that the render exists in a hidden state and then comes in once we know the users status.
4. We have a fallback or blank on the ad system so that space is always used

I will play around with it a bit and see what looks nicest.

Showing a medium rectangle ad format above and below the fold for mobile. I took a crack at it, but it doesn't request different placements when I use browser tools to emulate mobile viewports. Should I be pulling in the useWindowDimensions hook for this?

I think we just need to set up some hooks for this to work. We do have that viewport provider ... but full transparency, I dislike it and have wanted to remove it multiple times, I think there is a better way to manage it. I can fiddle with that as well.

Test coverage. I'd love some guidance on where I can add tests for this new stuff.

So we generally rely on snapshot testing for smoke tests on rendered pages... and even more so in the realm of third party integrations (since the assumption is testing is the domain of the library). For this we would really just test that things don't fall to pieces when the lib fails and make sure we have graceful fallbacks. Testing has a long sordid history with Pocket that has been a tale of woe for me that I am happy to share if you are ever interested.

[mashalifshin]

I took a quick look at the snapshots and it seems like we snapshot test at the level of components here? So I saw that Placard has a snapshot, so maybe snapshot testing the content-ads components (BillboardAboveTheFold & co, and/or AdSlot?) would be the way to go? Lmk if that makes sense and/or you have other ideas

Testing has a long sordid history with Pocket that has been a tale of woe for me that I am happy to share if you are ever interested.

I would like to subscribe to Testing Tales of Woe with Joel, lol. Want to 🍐 ? Or can subscribe in Slack of course