Capture globals generically #719
Conversation
jonathanKingston
force-pushed
the
jkt/type-globals
branch
from
39fd905
to
6bc060d
Compare
There was a problem hiding this comment.
@jonathanKingston I know we've been over this problem a few times and this implementation looks fine from a code POV.
The only reason I didn't give an explicit approval is based on not knowing the scope of testing before merging? For instance, is this something you intend to test cross-platform before we merge, or did you want to cut a release and test as each platforms updates?
I don't have any preferences here, just curious what the plan is there (if you have one yet :))
| @@ -89,7 +89,7 @@ function injectFeatures (args) { | |||
| ${codeFeatures.join('\n')} | |||
| })();` | |||
| script.src = 'data:text/javascript;base64,' + btoa(code) | |||
| getInjectionElement().appendChild(script) | |||
| getInjectionElement()?.appendChild(script) | |||
There was a problem hiding this comment.
Was this just noticed during testing and fixed on the fly?
There was a problem hiding this comment.
I changed the method signature to return null also.
The reason this was added is the utils.js is used within the extension testing which doesn't have a DOM, so the code falls back to just using the existing global (under a different alias so still some slight improvements).
There's been a fair amount of testing on each platform locally, some changes have happened since though so happy to just verify as we roll out. |
jonathanKingston
force-pushed
the
jkt/type-globals
branch
from
ef18595
to
c45f6fa
Compare
|
Marking as on draft as on hold right now. |
See: https://app.asana.com/0/72649045549333/1204999114961040/f
Also resolves the core issue of: https://app.asana.com/0/892838074342800/1205340475534185/f