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

#7255: Remove data-pb-ready attribute #8120

Merged
merged 4 commits into from
Apr 1, 2024

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Mar 31, 2024

In general, the DOM is hostile ground because the host page can tamper with it. - Todd

What does this PR do?

Checklist

  • Add tests and/or storybook stories
  • Designate a primary reviewer: @grahamlangford

@@ -19,7 +19,6 @@ const knipConfig = {
"static/*",

// App messenger and common storage
"src/contentScript/externalProtocol.ts",
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 was the content-script-based messenger we had to implement because Firefox lacks onConnectExternal

Should have been dropped with

Dropped here because it awaited the data-pb-ready attribute


onContextInvalidated.addListener(() => {
unsetReadyInThisDocument(uuid);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refer to the updated @file notes.

In short, "readiness" is not part of the document anymore, so it does not need to be unset. Each content script remains forever "ready" yet unconnectable.

export function isInstalledInThisSession(): boolean {
return CONTENT_SCRIPT_INJECTED_SYMBOL in globalThis;
export function isContentScriptInstalled(): boolean {
return CONTENT_SCRIPT_INJECTED_SYMBOL in window;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No globalThis support for symbols in TypeScript. This preserves consistency with the set* functions below

@fregante fregante marked this pull request as ready for review March 31, 2024 19:13
Copy link

codecov bot commented Mar 31, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 73.29%. Comparing base (71182b2) to head (39b299f).

Files Patch % Lines
src/contentScript/ready.ts 55.55% 4 Missing ⚠️
src/contentScript/contentScript.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    pixiebrix/pixiebrix-extension#8120      +/-   ##
==========================================
+ Coverage   73.19%   73.29%   +0.10%     
==========================================
  Files        1310     1309       -1     
  Lines       40732    40670      -62     
  Branches     7569     7558      -11     
==========================================
- Hits        29814    29811       -3     
+ Misses      10918    10859      -59     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@fregante fregante marked this pull request as draft April 1, 2024 07:18
@fregante
Copy link
Contributor Author

fregante commented Apr 1, 2024

Tested, I didn't see any issues with the page editor, sidebar, and quickbar. Could not test the app communication because my dog ate my password I lost the password 🫡

@fregante fregante marked this pull request as ready for review April 1, 2024 07:30
@fregante fregante merged commit fa97566 into main Apr 1, 2024
26 checks passed
@fregante fregante deleted the F/feature/drop-document-ready-attribute branch April 1, 2024 07:31
@twschiller twschiller added this to the 1.8.12 milestone Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants