Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Migrate SVG to JS types #40401

Merged
merged 2 commits into from
Mar 22, 2023
Merged

Migrate SVG to JS types #40401

merged 2 commits into from
Mar 22, 2023

Conversation

joshualitt
Copy link
Contributor

No description provided.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 17, 2023
@joshualitt joshualitt marked this pull request as ready for review March 17, 2023 22:21
@joshualitt
Copy link
Contributor Author

@eyebrowsoffire ptal. @ditman, @mdebbar FYI, please feel free to review if you like.

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM if @eyebrowsoffire likes it.

@@ -10,7 +11,8 @@ import 'package:ui/src/engine.dart' as engine;
import 'package:ui/ui.dart' as ui;

@JS('_flutter')
external set loader(Object? loader);
external set _loader(JSAny? loader);
set loader(Object? l) => _loader = l?.toJSAnyShallow;
Copy link
Member

Choose a reason for hiding this comment

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

Took me a while to figure out why this was a setter (it's a test 🤦)

Copy link
Contributor

Choose a reason for hiding this comment

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

I may be misunderstanding something here, but isn't this redundant with the js_util.jsify(...) calls below?

set loader(Object? l) => _loader = l?.toJSAnyShallow;

// in tests:
loader = js_util.jsify(<String, Object>{
  'loader': <String, Object>{
    'didCreateEngineInitializer': allowInterop(() { print('not mocked'); }),
  },
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yea, good catch, it is redundant. I'm not actually trying to investigate usage patterns too deeply at this phase of the migration, mostly to derisk the conversion. That said, this is a good candidate for cleanup when we try to push JS types deeper into the Web engine. Thankfully this is a test, and jsify now has fast paths for types it doesn't need to convert.

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

Looks great! I have one comment... it's not necessarily blocking but it would be really nice if you had the time to do it.

@joshualitt joshualitt force-pushed the 2-js-types branch 3 times, most recently from ef4ac03 to 2943832 Compare March 21, 2023 21:53
@joshualitt joshualitt merged commit 5775c6b into flutter:main Mar 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 22, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 22, 2023
Roll Flutter Engine from bb51cda2fa47 to 5775c6b05fa4 (1 revision)
sourcegraph-bot pushed a commit to sgtest/megarepo that referenced this pull request Mar 22, 2023
…23217)

Commit: b42e8db6cf8f7aa0e8debb7dec423b9080d3727f
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants