-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[web] allow imports to line-break #57170
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM
web_sdk/sdk_rewriter.dart
Outdated
@@ -79,6 +79,7 @@ List<Replacer> generatePartsPatterns(String libraryName, bool isPublic) { | |||
// Remove library directives. | |||
AllReplacer(RegExp(r'\nlibrary .+;'), ''), | |||
// Remove imports/exports from all part files. | |||
AllReplacer(RegExp(r"\nimport '.+'\n\s*if \(dart.library.html\) '.*';"), ''), |
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.
can we not hardcode dart.library.html
or maybe also allow for dart.library.js_interop
as well, so our code doesn't mysteriously break when we migrate the code to not referencing dart.library.html? Bugs in the sdk rewriter are very hard to pinpoint :/
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.
Done.
045cd24
to
81816cc
Compare
Looks like something is still failing with the line breaks… |
@yjbanov I think we need to remove this restriction now: engine/web_sdk/test/js_access_test.dart Lines 184 to 187 in f0ff4f2
(this is what's causing the CI failure) |
Yep, fixed. |
We are about to
dart format
all the Dart code in the repo, which line-breaks some of our imports.This PR updates
sdk_rewriter.dart
to support line-broken imports.