Support package: URLs in Dart #53

luisvt · 2016-12-01T17:32:12Z

fixes #19

nex3 · 2016-12-02T21:29:57Z

I like this change, but I think we should take a SyncPackageResolver rather than a raw map. A resolver is a lot easier to get for the current isolate, and it's more flexible in case the user wants to use packages/ directories for whatever reason.

luisvt · 2016-12-02T23:13:39Z

Using SyncPackageResolverer inside visitor will make the application not compilable to js because this depends on dart:io. That's why I created a separated file for handling the PackageResolver.

nex3 · 2016-12-06T21:39:33Z

We use configuration-specific imports to support VM-only APIs as well as JS. You can add a conditional import that imports a stubbed SyncPackageResolver with only the APIs we use when compiling to JS.

luisvt · 2016-12-08T05:34:07Z

ok, I'll try to make the changes. I'll let you know

luisvt · 2016-12-18T19:37:06Z

Changes has been made. Please review them @nex3

nex3 · 2016-12-19T01:10:38Z

lib/sass.dart

@@ -15,12 +16,12 @@ import 'src/visitor/serialize.dart';
 /// If [color] is `true`, this will use terminal colors in warnings.
 ///
 /// Throws a [SassException] if conversion fails.
-String render(String path, {bool color: false}) {
+Future<String> render(String path, {bool color: false}) async {


This function should take the SyncPackageResolver, not _PerformVisitor. Rendering needs to be synchronous in order to be compatible with the node-sass API.

Also, there should be a command-line --packages flag that allows the user to manually specify a package spec.

what do you mean with package spec?

A .packages file.

nex3 · 2016-12-19T01:11:30Z

lib/src/parse/stylesheet.dart

@@ -1403,6 +1403,12 @@ abstract class StylesheetParser extends Parser {
        break;

      case $0:
+        if (scanner.peekChar(1) == $backslash && scanner.peekChar(2) == $0) {


What's this all about?

these lines are for parsing code like next:

@media screen and (min-width:0\0) { ... }

specifically are for parsing 0\0 inside a media query.

Please leave fixes for other issues outside of this PR. One thing at a time makes it easier to review.

nex3 · 2016-12-19T01:11:54Z

lib/src/sync_package_resolver/interface.dart

+/// [PackageInfo] is usually preferable when the current Isolate's package
+/// resolution logic may be used.
+///
+/// This class should not be implemented by user code.


You don't need to copy over the documentation here or in the stub implementation.

nex3 · 2016-12-19T01:15:05Z

lib/src/sync_package_resolver/node.dart

+  /// Returns `null` when using a [packageRoot] for resolution, or when no
+  /// package resolution is being used.
+  Map<String, Uri> get packageConfigMap =>
+      throw new Exception('not implemented for node');


These methods should throw UnsupportedExceptions, which should include the name of the class and method (for example, "SyncPackageResolver.packageConfigMap isn't supported on Node.js.").

nex3 · 2016-12-19T01:18:06Z

lib/src/visitor/perform.dart

@@ -509,11 +519,17 @@ class _PerformVisitor
    });
  }

+  Uri _resolvePackageUri(Uri packageUri) {
+    return packageUri.scheme == 'package' && packageResolver != null
+        ? packageResolver.resolveUri(packageUri) ?? new Uri()


I don't think new Uri() is right here. A blank URL doesn't help anyone. We should throw a useful error message saying "Can't resolve "$packageUri"" if it fails to resolve, or if there's no resolver available.

nex3 · 2016-12-19T01:20:23Z

node_sample/import-file/_variables.scss

@@ -0,0 +1 @@
+$color: blue;


What is this node_sample directory for?

nex3 · 2016-12-19T01:24:49Z

test/import_package_uri_test.dart

+main() {
+  group('import "package:" uri', () {
+    test('found', () async {
+      var result = await render('test/styles/import-package.scss');


Rather than using assets on the filesystem, Dart Sass tests use scheduled_test's descriptor library to create assets in the test itself. This helps keep the test self-contained, and makes diffs a lot easier to read. See test/cli_shared.dart for examples.

nex3 · 2016-12-19T01:26:22Z

pubspec.yaml

@@ -29,3 +30,4 @@ dev_dependencies:
  node_preamble: "^1.0.0"
  test: "^0.12.0"
  yaml: "^2.0.0"
+  bootstrap_sass: ^4.0.0-alpah.3+4


I don't like making assertions about other packages' assets in tests. I'd rather create a folder in the test body, then create a .packages file that points to it.

nex3 · 2016-12-19T01:28:00Z

tool/grind.dart

@@ -16,15 +15,15 @@ import 'package:yaml/yaml.dart';

 /// The version of Dart Sass.
 final String _version =
-    loadYaml(new File('pubspec.yaml').readAsStringSync())['version'];
+    loadYaml(new File('pubspec.yaml').readAsStringSync())['version'] as String;


It'll be quicker for me to review this PR if you keep the changes focused on package: URLs.

nex3 · 2017-01-06T23:22:10Z

@luisvt Is this ready for another round of review?

luisvt · 2017-01-07T06:38:23Z

Not yet sorry

googlebot · 2017-01-10T16:49:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2017-01-10T16:58:02Z

CLAs look good, thanks!

luisvt · 2017-01-10T17:12:35Z

now I think is ready, please review it.

nex3 · 2017-01-10T22:05:37Z

lib/sass.dart

+/// If [color] is `true`, this will use terminal colors in warnings.
+///
+/// Throws a [SassException] if conversion fails.
+Future<String> renderAsync(String path, {bool color: false}) async =>


I don't think I want to add this. I don't think having a default for packageResolver is worth an extra top-level function.

nex3 · 2017-01-10T22:06:47Z

lib/src/node.dart

-void _render(
-    NodeOptions options, void callback(NodeError error, NodeResult result)) {
+Future _render(NodeOptions options,
+    void callback(NodeError error, NodeResult result)) async {


Why are you changing the types here? Node doesn't understand what a "Future" is, so this will definitely break it.

Sorry, I forgot to revert it.

nex3 · 2017-01-10T22:08:15Z

lib/src/sync_package_resolver/index.dart

@@ -0,0 +1,3 @@
+export 'interface.dart'


To match the way we stub out IO, this file (sync_package_resolver/index.dart) should be sync_package_resolver.dart instead.

nex3 · 2017-01-10T22:09:21Z

lib/src/sync_package_resolver/interface.dart

+import 'dart:async';
+
+abstract class SyncPackageResolver {
+  Map<String, Uri> get packageConfigMap;


Only include the members that Dart Sass actually uses in practice.

nex3 · 2017-01-10T22:12:19Z

lib/src/visitor/perform.dart

@@ -123,8 +128,13 @@ class _PerformVisitor
  /// invocations, and imports surrounding the current context.
  final _stack = <Frame>[];

+  final SyncPackageResolver packageResolver;


Make this private.

nex3 · 2017-01-10T22:13:02Z

lib/sass.dart

@@ -15,12 +17,24 @@ import 'src/visitor/serialize.dart';
 /// If [color] is `true`, this will use terminal colors in warnings.
 ///
 /// Throws a [SassException] if conversion fails.
-String render(String path, {bool color: false}) {
+String render(String path,
+    {bool color: false, SyncPackageResolver packageResolver}) {


Document the packageResolver parameter.

nex3 · 2017-01-10T22:15:13Z

lib/src/visitor/perform.dart

+  Uri _resolvePackageUri(DynamicImport import) {
+    var packageUri = import.url;
+    if (packageUri.scheme == 'package') {
+      if (packageResolver == null)


Nit: include {} even for single-statement ifs (see the style guide).

nex3 · 2017-01-10T22:15:56Z

pubspec.yaml

@@ -19,6 +19,7 @@ dependencies:
  string_scanner: ">=0.1.5 <2.0.0"
  stack_trace: ">=0.9.0 <2.0.0"
  tuple: "^1.0.0"
+  package_resolver: ^1.0.2


Always declare the lowest version that has all the APIs you need. In this case, that's 1.0.0.

nex3 · 2017-01-10T22:21:21Z

test/dart_cli_test.dart

+
+  group("supports package imports", () {
+    test("is found", () {
+      d.file("test.scss", "a {b: 1 + 2}").create('lib');


Don't create files outside of the sandbox during tests. To properly test this, you'll probably need to add that --packages flag.

nex3 · 2017-01-10T22:28:49Z

lib/src/executable.dart

@@ -47,7 +47,7 @@ main(List<String> args) async {

  var color = (options['color'] as bool) ?? hasTerminal;
  try {
-    var css = render(options.rest.first, color: color);
+    var css = await renderAsync(options.rest.first, color: color);


Defaulting to the current isolate's package resolver only makes sense here if Sass is being used as a library or a direct dependency of a package. If it's being run as a global or standalone executable—which is better supported anyway—that logic won't work.

Probably the best thing to do here is to follow Dart's behavior for auto-detecting package resolution from the Sass file's location, and allow users to override it with a --packages flag. For now, just remove the default behavior and implement the flag; we can do the auto-detection in a separate CL.

nex3

This is looking really good! Just a few small changes left.

nex3 · 2017-01-13T21:33:40Z

lib/src/visitor/perform.dart

+
+      var resolvedPackageUri = _packageResolver.resolveUri(packageUri);
+      if (resolvedPackageUri == null ||
+          !dirExists(p.dirname(p.fromUri(resolvedPackageUri)))) {


I'm not sure I understand the point of this check. If the package URI is resolvable, we'll already get an error when we try to load the file, right?

This check is for when the package URI is not resolvable. What should I do instead this check?

If I remove this check resolvedPackageUri will be null, which in turn will throw:

NoSuchMethodError: The getter 'scheme' was called on null.

Second option would be return an empty uri, which was what I have at the beginning and you told me to better add a check and throw a better error. Please see the comment you added on #53 (diff)

Third option would be return the original packageUri, but that lets to previous problem on having an error message not showing good information.

Finally the line 585 in #53 (diff) was added because previous version of sdk returned an URI even when there was no existing file.

nex3 · 2017-01-13T21:34:14Z

test/dart_cli_test.dart

+//      ]));
+//      sass.shouldExit(0);
+//    });
+//  });


Remove this commented-out code.

nex3 · 2017-01-13T21:37:35Z

test/dart_script_test.dart

+          "@import \"package:fake_package/test_aux\";\n"
+          "        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n"
+          "  $sandbox/test.scss 1:9  root stylesheet");
+    }


It's generally better to use the throwsA() matcher than to manually write a try/catch block. Then you can verify the exception's message with the predicate() matcher.

changes: * remove commented out code in `dart_cli_test.dart` * use `throwsA` instead doing a `try-catch` in `dart_script_test.dart`

googlebot · 2017-01-27T03:19:56Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2017-01-27T03:30:52Z

CLAs look good, thanks!

googlebot · 2017-01-27T03:44:21Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

changes: * remove `dirExist` check for `packageUri`

googlebot · 2017-02-03T08:29:37Z

CLAs look good, thanks!

Exporting a stub produced errors when passing a real instance.

nex3 · 2017-02-03T09:08:16Z

Thanks for the patch!

Co-authored-by: Natalie Weizenbaum <nweiz@google.com>

luisvt force-pushed the support-package-url branch from 8fbcb0c to 6b30c9c Compare December 1, 2016 17:37

luisvt force-pushed the support-package-url branch 4 times, most recently from b8509ca to a32d759 Compare December 15, 2016 06:30

nex3 requested changes Dec 19, 2016

View reviewed changes

luisvt force-pushed the support-package-url branch from a32d759 to 270551c Compare December 31, 2016 21:14

luisvt mentioned this pull request Jan 4, 2017

@import inside comments should not get processed dart-league/dart-sass#6

Closed

luisvt force-pushed the support-package-url branch 12 times, most recently from 7935021 to 93a5d83 Compare January 4, 2017 07:03

luisvt force-pushed the support-package-url branch from 93a5d83 to b6095cc Compare January 10, 2017 16:49

Support "package:" URLs

1f33413

luisvt force-pushed the support-package-url branch from b6095cc to 7dcb1c8 Compare January 10, 2017 16:58

Support "package:" URLs

40c7950

luisvt force-pushed the support-package-url branch from 7dcb1c8 to 40c7950 Compare January 10, 2017 17:08

nex3 requested changes Jan 10, 2017

View reviewed changes

Support "package" uri.

1168754

luisvt force-pushed the support-package-url branch from 0d6e758 to 1168754 Compare January 11, 2017 04:24

nex3 requested changes Jan 13, 2017

View reviewed changes

Support "package" uri.

87934d3

changes: * remove commented out code in `dart_cli_test.dart` * use `throwsA` instead doing a `try-catch` in `dart_script_test.dart`

luisvt force-pushed the support-package-url branch from e33a390 to 87934d3 Compare January 27, 2017 03:30

Support "package" uri.

880de11

changes: * remove `dirExist` check for `packageUri`

luisvt force-pushed the support-package-url branch from 2eaddcd to 880de11 Compare January 27, 2017 03:45

Style and prose changes.

08e0788

nex3 force-pushed the support-package-url branch from 1102235 to 08e0788 Compare February 3, 2017 08:38

Export the real SyncPackageResolver, not a stub.

8c35df7

Exporting a stub produced errors when passing a real instance.

nex3 merged commit cfc3a15 into sass:master Feb 3, 2017

zoechi mentioned this pull request Feb 4, 2017

Add information on how to use the new package in a dart project #114

Closed

zoechi mentioned this pull request Mar 14, 2017

Support package: imports when using the executable #128

Open

nex3 added a commit that referenced this pull request May 10, 2023

Implement --version flag for cli (#53)

d206351

Co-authored-by: Natalie Weizenbaum <nweiz@google.com>

Support package: URLs in Dart #53

Support package: URLs in Dart #53

Conversation

luisvt commented Dec 1, 2016

nex3 commented Dec 2, 2016

luisvt commented Dec 2, 2016

nex3 commented Dec 6, 2016

luisvt commented Dec 8, 2016

luisvt commented Dec 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 commented Jan 6, 2017

luisvt commented Jan 7, 2017

googlebot commented Jan 10, 2017

googlebot commented Jan 10, 2017

luisvt commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jan 27, 2017

googlebot commented Jan 27, 2017

googlebot commented Jan 27, 2017

googlebot commented Feb 3, 2017

nex3 commented Feb 3, 2017