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.

Sign up for GitHub

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

Jump to bottom

refactor(content): simplify MonetizationLinkManager #538

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

refactor(content): simplify MonetizationLinkManager #538

wants to merge 41 commits into from

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Aug 23, 2024 •
edited
Loading

Context

Closes #455

Changes proposed in this pull request

  • Rename MonetizationTagManager to MonetizationLinkManager (as we want to handle only links in this service)
  • Simplify and reorganize MonetizationLinkManager
  • TypeScript improvements (check more things)

@github-actions github-actions bot added the area: content Improvements or additions to extension content script label Aug 23, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 23, 2024 •
edited
Loading

Extension builds preview

Name Link
Latest commit 7ba33cc
Latest job logs Run #10848861748
BadgeDownload
BadgeDownload

@github-actions github-actions bot added the area: background Improvements or additions to extension background script label Aug 26, 2024
@sidvishnoi
remove todo notes/comments
2df5eb6
@sidvishnoi
remove unused types
038cfed
@sidvishnoi
bring in iframe support with existing frameMananger
bfa8d64
@sidvishnoi
remove monetizationTagManager; use monetizationLinkManager
c53b7e1
@sidvishnoi
update payload types (extract *PayloadEntry out)
b192f05
@sidvishnoi
nit: extract postMessage call to private method
5885b5f
@sidvishnoi
add types for Window.postMessage messages
1a9920e
@sidvishnoi
nit
d1e837f
@sidvishnoi
implement MonetizationLinkManager.end (unlisten everything)
0bea75c
@sidvishnoi
don't define polyfill again on re-inject
8bf32a2 
Relatively safe to do as we don't update it often. Would require page
reload otherwise - which can lead to data loss. "undefining" properties
is also not an option - as otherwise any script on page on reconfigure
them.
@sidvishnoi
cleanup MonetizationLinkManager on disconnect
76f2e92 
Prevents Extension context invalidated error
(at least I don't see it in console anymore)
raducristianpopa
raducristianpopa reviewed Sep 2, 2024
View reviewed changes
Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

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

Only code review for now - will have to test the extension as well.

src/content/services/monetizationLinkManager.ts Outdated Show resolved Hide resolved
src/content/services/monetizationLinkManager.ts Outdated Show resolved Hide resolved
src/content/services/monetizationLinkManager.ts Outdated Show resolved Hide resolved
src/content/services/monetizationLinkManager.ts Outdated Show resolved Hide resolved
src/content/services/frameManager.ts Outdated Show resolved Hide resolved
src/content/polyfill.ts Outdated Show resolved Hide resolved
@sidvishnoi sidvishnoi marked this pull request as ready for review September 6, 2024 11:06
raducristianpopa
raducristianpopa reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@raducristianpopa raducristianpopa 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 the Monetization event is not sent to links inside iframes nor we send the START_MONETIZATION message to background from an iframe?

load event is sent on the other hand - the link is being validated.

raducristianpopa
raducristianpopa approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

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

Before merging this, let's get some feedback from other persons as well to check if this is working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raducristianpopa raducristianpopa raducristianpopa approved these changes

Assignees
No one assigned
Labels
area: background Improvements or additions to extension background script area: content Improvements or additions to extension content script
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Monetization tag manager refactoring
2 participants
@sidvishnoi @raducristianpopa