-
Notifications
You must be signed in to change notification settings - Fork 79
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
Migrate injected client to package web #2306
Migrate injected client to package web #2306
Conversation
looks like you need to align the pubspec w/ the changelog version |
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.
Changes look good! I have mostly nits.
@srujzs I addressed all the comments and made sure the code works internally and externally. Appreciate another look! |
|
||
extension SdkExtension on Sdk { | ||
JSObject get dart => (this as JSObject)['dart'] as JSObject; | ||
JSObject get developer => (this as JSObject)['developer'] as JSObject; |
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.
nit: I should've caught this when we were debugging together, but alternatively:
external JSObject get dart;
external JSObject get developer;
external JSObject get _extensions;
does the same thing. :)
One last nit, but LGTM! Thanks again for all your work here! |
@srujzs Removed all uses of unsafe js interop:) Appreciate another look! |
Woohoo, no more |
Superseded by a5ac4ac \cc @derekxu16 for closing |
Migrate injected client code to
package:web
.Closes: #2338