-
Notifications
You must be signed in to change notification settings - Fork 5
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
AF-1837 dart 1 and dart2 compat #38
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on HipChat: InfoSec Forum. |
Dockerfile
Outdated
tar czvf font_face_observer.pub.tgz LICENSE README.md pubspec.yaml lib/ && \ | ||
./tool/codecov.sh && \ | ||
echo "Script sections completed" | ||
pub get && \ |
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.
remove --packages-dir
Dockerfile
Outdated
pub publish --dry-run && \ | ||
pub run dart_dev analyze && \ | ||
pub run dart_dev format --check && \ | ||
xvfb-run -s '-screen 0 1024x768x24' pub run dart_dev test --web-compiler=dartdevc -p chrome && \ |
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.
removed coverage since there isn't a way to collect coverage under dart 2 yet
import 'dart:html'; | ||
import 'dart:typed_data'; | ||
import 'package:dart2_constant/convert.dart' as convert; |
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.
use dart2_constant to use dart 1&2 compatible constant names
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.
For my own edification, is there a reference you can provide explaining this change?
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.
There's a library that allows you to use constants that work under both dart 1 and dart 2 https://pub.dartlang.org/packages/dart2_constant
If we didn't care about backwards compatibility, we could just move to dart 2 camelcase constants by running https://pub.dartlang.org/packages/dart2_fix
@@ -66,7 +66,7 @@ https://www.w3.org/Consortium/Legal/2015/copyright-software-and-document | |||
const String adobeBlankFamily = 'AdobeBlank'; | |||
|
|||
/// The font key for the AdobeBlank font | |||
final String adobeBlankKey = '${adobeBlankFamily}_normal_normal_normal'; | |||
const String adobeBlankKey = '${adobeBlankFamily}_normal_normal_normal'; |
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.
lint
test/font_face_observer_test.dart
Outdated
@@ -216,7 +216,7 @@ void main() { | |||
|
|||
|
|||
test('should keep data-uses attribute up to date', () async { | |||
final String differentGroup = 'diff'; | |||
const String differentGroup = 'diff'; |
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.
lint ^
border: solid 1px #ccc; | ||
border-collapse: collapse; | ||
padding: 5px; | ||
} |
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.
just format changes
@@ -25,7 +30,7 @@ | |||
<th>DOM</th> | |||
<th>Canvas</th> | |||
</tr> | |||
<script type="application/dart" src="main.dart"></script> | |||
<script src="../packages/browser/dart.js"></script> | |||
<script defer src="main.dart.js"></script> |
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.
get rid of transformers, browser package according to https://webdev-dartlang-org-dev.firebaseapp.com/dart-2
..text = message | ||
..style.fontFamily = uniqFamily | ||
..style.fontSize = '18px')) | ||
..append(new TableCellElement()..append(canvas)); |
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.
format
@@ -30,7 +30,6 @@ <h1>Memory / Perf Analysis</h1> | |||
<div style="font-family: Roboto_2, Times New Roman">Roboto_2 group_A & group_B</div> | |||
<div style="font-family: Roboto_3, Times New Roman">Roboto_3 group_A</div> | |||
|
|||
<script type="application/dart" src="unload.dart"></script> | |||
<script src="../packages/browser/dart.js"></script> | |||
<script defer src="unload.dart.js"></script> |
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 for Dart 2 / DDC .. no rewriting, no transformers
Dockerfile
Outdated
export D2PATH=`pwd`/dart-sdk/bin && \ | ||
export PATH=$D2PATH:$PATH && \ | ||
dart --version && \ | ||
pub --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.
Download dart 2 dev, extract and add it to the path to use dart 2 in CI. Maybe we could include both versions of Dart this way in CI images include a quick command to switch versions easily?
@@ -67,10 +67,14 @@ import 'package:font_face_observer/font_face_observer.dart'; | |||
|
|||
String _groupA = 'group_A'; | |||
String _groupB = 'group_B'; | |||
_FontConfig _cfg = new _FontConfig(family: 'Roboto_1', url: '/fonts/Roboto.ttf', useSimulatedLoadEvents: true); |
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.
move to example dir, reformatted
final String stretch = 'my stretch'; | ||
final String testString = 'my testString'; | ||
final int timeout = 1337; | ||
const String family = 'my family'; |
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.
Is const
the new Dart 2 preferred?
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.
Yes, Dart 2 now recommends you use const in certain places. It isn't an error, just a lint.
QA +1 / +10
Merging into master. |
Overview
Prepare the code and process to work under both dart 1 and dart 2
Testing