-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
refactor: make inpage.js
inject itself into the MAIN world (manifest v2 only)
#22524
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Added team-security label to check out the reintroduction of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given some of my notes, we should discuss how to proceed
2ea8dfa
to
ea04f8c
Compare
…-extension into refactor-inject-inpage
src
in manifest v22358cc2
to
0072a28
Compare
inpage.js
inject itself into the MAIN world (manifest v2 only)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #22524 +/- ##
========================================
Coverage 68.45% 68.45%
========================================
Files 1089 1089
Lines 42902 42902
Branches 11428 11428
========================================
Hits 29368 29368
Misses 13534 13534 ☔ View full report in Codecov by Sentry. |
@davidmurdoch they are precise but not accurate, if in general over the course of many builds the value is higher then it represents an actionable issue. |
Builds ready [e5f2b68]
Page Load Metrics (798 ± 25 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the history of inpage.js goes back to the beginning of time so it's possible browsers have changed their behavior that motivated the current setup. I believe our reasonings included:
- inpage.js execution timing. our current setup was the only way we found to run before page code did
- ability to modify the page globalThis to add the api. I thought contentscript couldn't do this, only modify the DOM.
- clear separation of security sensitive (content script) and non-sensitive (inpage.js)
- bundle factoring inpage should be a single bundle and not factored into multiple files
some of your changes here may not have affected the above
its the same set up, but inpage.js now injects itself.
inpage.js now modifies the DOM to inject itself into the DOM so it can modify
they are now more separated, as contentscript.js doesn't depend on inpage.js now(and visa versa)
no changes here |
Builds ready [a83bff8]
Page Load Metrics (807 ± 38 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, few notes to discuss ☝️
Co-authored-by: weizman <weizmangal@gmail.com>
Co-authored-by: weizman <weizmangal@gmail.com>
Builds ready [2861767]
Page Load Metrics (732 ± 17 ms)
Bundle size diffs
|
Builds ready [9ae02ae]
Page Load Metrics (810 ± 25 ms)
Bundle size diffs
|
Description
I'm working on a new build process that is primary focused on compilation speed and simplicity. The current manifest v2 way of injecting inpage.js makes that task harder.
Affects manifest v2 only.
This PR changes the way inpage.js is injected into the MAIN world by decoupling
inpage.js
fromcontentscript.js
.This new way is very similar to the old way, except
inpage.js
now injects itself into the document (MAIN world).One potential issue with this new way is that
contentscript.js
used to conditionally injectinpage.js
into the MAIN world, but with this change theinpage.js
file is just another "content_script". This might not be a problem though, asinpage.js
itself checks all of the same conditionscontentscript.js
does, i.e., they both checkshouldInjectProvider()
, but the difference is that onlyinpage.js
decides itself if it will inject a provider into the MAIN world, whereas before it was up to both contentscript.js AND inpage.js to agree to inject the provider.To reiterate why I want to make this change: the current system requires custom logic specifically for
contentscript.js
andinpage.js
, and inpage.js MUST complete compilation beforecontentscript.js
can be start to be compiled. Additionally,contentscript.js
needs special treatment offs.readFileSync
in order to inlineinpage.js
as text into itself. This complicates and slows down the build system.One thing this PR does NOT do is speed up the build, as I didn't change the order of compilation.
inpage.js
is still compiled beforecontentscript.js
, even thoughcontentscript.js
no longer depends oninpage.js
. I a) didn't know how to make this change, and b) was afraid it'd complicate an already perhaps dubious PR.Some things to look out for in testing: will this new way behave if the page is not an HTML doctype, is an XML/PDF document, doesn't have a
documentElement
, is on the blocked domains list, etc.Related issues
Fixes:
Manual testing steps
window.ethereum
and press in the ConsoleProxy
object should be logged (notundefined
)Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist