From 3a493f23ab8cb1ec0fc69c447fcd207743439213 Mon Sep 17 00:00:00 2001 From: Nicholas Shahan Date: Thu, 20 Dec 2018 16:08:26 -0800 Subject: [PATCH 1/3] Add documentation comment collection to AST (#548) - Documentation comments are simply identified by starting with a triple-slash "///". - Adjacent comments in indented syntax are now coalesced into a single comment. --- .../sass/statement/callable_declaration.dart | 10 +- lib/src/ast/sass/statement/function_rule.dart | 6 +- lib/src/ast/sass/statement/mixin_rule.dart | 5 +- .../ast/sass/statement/silent_comment.dart | 21 ++ .../sass/statement/variable_declaration.dart | 9 +- lib/src/parse/sass.dart | 52 +++-- lib/src/parse/scss.dart | 3 +- lib/src/parse/stylesheet.dart | 17 +- test/doc_comments_test.dart | 200 ++++++++++++++++++ 9 files changed, 294 insertions(+), 29 deletions(-) create mode 100644 test/doc_comments_test.dart diff --git a/lib/src/ast/sass/statement/callable_declaration.dart b/lib/src/ast/sass/statement/callable_declaration.dart index 2e8b42d5..f1a6eccd 100644 --- a/lib/src/ast/sass/statement/callable_declaration.dart +++ b/lib/src/ast/sass/statement/callable_declaration.dart @@ -7,6 +7,7 @@ import 'package:source_span/source_span.dart'; import '../argument_declaration.dart'; import '../statement.dart'; import 'parent.dart'; +import 'silent_comment.dart'; /// An abstract class for callables (functions or mixins) that are declared in /// user code. @@ -16,12 +17,17 @@ abstract class CallableDeclaration extends ParentStatement { /// This may be `null` for callables without names. final String name; + /// The comment immediately preceding this declaration. + final SilentComment comment; + /// The declared arguments this callable accepts. final ArgumentDeclaration arguments; final FileSpan span; CallableDeclaration( - this.name, this.arguments, Iterable children, this.span) - : super(List.unmodifiable(children)); + this.name, this.arguments, Iterable children, this.span, + {SilentComment comment}) + : comment = comment, + super(List.unmodifiable(children)); } diff --git a/lib/src/ast/sass/statement/function_rule.dart b/lib/src/ast/sass/statement/function_rule.dart index 707b671c..78bcd4d9 100644 --- a/lib/src/ast/sass/statement/function_rule.dart +++ b/lib/src/ast/sass/statement/function_rule.dart @@ -8,14 +8,16 @@ import '../../../visitor/interface/statement.dart'; import '../argument_declaration.dart'; import '../statement.dart'; import 'callable_declaration.dart'; +import 'silent_comment.dart'; /// A function declaration. /// /// This declares a function that's invoked using normal CSS function syntax. class FunctionRule extends CallableDeclaration { FunctionRule(String name, ArgumentDeclaration arguments, - Iterable children, FileSpan span) - : super(name, arguments, children, span); + Iterable children, FileSpan span, + {SilentComment comment}) + : super(name, arguments, children, span, comment: comment); T accept(StatementVisitor visitor) => visitor.visitFunctionRule(this); diff --git a/lib/src/ast/sass/statement/mixin_rule.dart b/lib/src/ast/sass/statement/mixin_rule.dart index ad7a5a1f..7256e92c 100644 --- a/lib/src/ast/sass/statement/mixin_rule.dart +++ b/lib/src/ast/sass/statement/mixin_rule.dart @@ -8,6 +8,7 @@ import '../../../visitor/interface/statement.dart'; import '../argument_declaration.dart'; import '../statement.dart'; import 'callable_declaration.dart'; +import 'silent_comment.dart'; /// A mixin declaration. /// @@ -23,8 +24,8 @@ class MixinRule extends CallableDeclaration { /// won't work correctly. MixinRule(String name, ArgumentDeclaration arguments, Iterable children, FileSpan span, - {this.hasContent = false}) - : super(name, arguments, children, span); + {this.hasContent = false, SilentComment comment}) + : super(name, arguments, children, span, comment: comment); T accept(StatementVisitor visitor) => visitor.visitMixinRule(this); diff --git a/lib/src/ast/sass/statement/silent_comment.dart b/lib/src/ast/sass/statement/silent_comment.dart index 7cffc2a7..be68207c 100644 --- a/lib/src/ast/sass/statement/silent_comment.dart +++ b/lib/src/ast/sass/statement/silent_comment.dart @@ -2,7 +2,10 @@ // MIT-style license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. +import 'dart:math' as math; + import 'package:source_span/source_span.dart'; +import 'package:string_scanner/string_scanner.dart'; import '../../../visitor/interface/statement.dart'; import '../statement.dart'; @@ -12,6 +15,24 @@ class SilentComment implements Statement { /// The text of this comment, including comment characters. final String text; + /// The subset of lines in text that are marked as part of the documentation + /// comments by beginning with '///'. + /// + /// The leading slashes and space on each line is removed. Returns `null` when + /// there is no documentation comment. + String get docComment { + var buffer = StringBuffer(); + for (var line in text.split('\n')) { + var scanner = StringScanner(line.trim()); + if (!scanner.scan('///')) continue; + scanner.scan(' '); + buffer.writeln(scanner.rest); + } + var comment = buffer.toString().trimRight(); + + return comment.isNotEmpty ? comment : null; + } + final FileSpan span; SilentComment(this.text, this.span); diff --git a/lib/src/ast/sass/statement/variable_declaration.dart b/lib/src/ast/sass/statement/variable_declaration.dart index ae807dd0..17925e50 100644 --- a/lib/src/ast/sass/statement/variable_declaration.dart +++ b/lib/src/ast/sass/statement/variable_declaration.dart @@ -9,6 +9,7 @@ import '../../../parse/scss.dart'; import '../../../visitor/interface/statement.dart'; import '../expression.dart'; import '../statement.dart'; +import 'silent_comment.dart'; /// A variable declaration. /// @@ -17,6 +18,9 @@ class VariableDeclaration implements Statement { /// The name of the variable. final String name; + /// The comment immediatly preceding this declaration. + SilentComment comment; + /// The value the variable is being assigned to. final Expression expression; @@ -33,9 +37,10 @@ class VariableDeclaration implements Statement { final FileSpan span; VariableDeclaration(this.name, this.expression, this.span, - {bool guarded = false, bool global = false}) + {bool guarded = false, bool global = false, SilentComment comment}) : isGuarded = guarded, - isGlobal = global; + isGlobal = global, + comment = comment; /// Parses a variable declaration from [contents]. /// diff --git a/lib/src/parse/sass.dart b/lib/src/parse/sass.dart index 3a600099..6df58fc4 100644 --- a/lib/src/parse/sass.dart +++ b/lib/src/parse/sass.dart @@ -185,28 +185,46 @@ class SassParser extends StylesheetParser { SilentComment _silentComment() { var start = scanner.state; scanner.expect("//"); - var buffer = StringBuffer(); var parentIndentation = currentIndentation; - while (true) { - buffer.write("//"); - // Skip the first two indentation characters because we're already writing - // "//". - for (var i = 2; i < currentIndentation - parentIndentation; i++) { - buffer.writeCharCode($space); + outer: + do { + var commentPrefix = scanner.scanChar($slash) ? "///" : "//"; + + while (true) { + buffer.write(commentPrefix); + + // Skip the initial characters because we're already writing the + // slashes. + for (var i = commentPrefix.length; + i < currentIndentation - parentIndentation; + i++) { + buffer.writeCharCode($space); + } + + while (!scanner.isDone && !isNewline(scanner.peekChar())) { + buffer.writeCharCode(scanner.readChar()); + } + buffer.writeln(); + + if (_peekIndentation() < parentIndentation) break outer; + + if (_peekIndentation() == parentIndentation) { + // Look ahead to the next line to see if it starts another comment. + if (scanner.peekChar(1 + parentIndentation) == $slash && + scanner.peekChar(2 + parentIndentation) == $slash) { + _readIndentation(); + } + break; + } + _readIndentation(); } + } while (scanner.scan("//")); - while (!scanner.isDone && !isNewline(scanner.peekChar())) { - buffer.writeCharCode(scanner.readChar()); - } - buffer.writeln(); - - if (_peekIndentation() <= parentIndentation) break; - _readIndentation(); - } - - return SilentComment(buffer.toString(), scanner.spanFrom(start)); + lastSilentComment = + SilentComment(buffer.toString(), scanner.spanFrom(start)); + return lastSilentComment; } /// Consumes an indented-style loud context. diff --git a/lib/src/parse/scss.dart b/lib/src/parse/scss.dart index b2b91d62..559d336b 100644 --- a/lib/src/parse/scss.dart +++ b/lib/src/parse/scss.dart @@ -158,8 +158,9 @@ class ScssParser extends StylesheetParser { scanner.spanFrom(start)); } - return SilentComment( + lastSilentComment = SilentComment( scanner.substring(start.position), scanner.spanFrom(start)); + return lastSilentComment; } /// Consumes a statement-level loud comment block. diff --git a/lib/src/parse/stylesheet.dart b/lib/src/parse/stylesheet.dart index c436c87b..ec4aa0e5 100644 --- a/lib/src/parse/stylesheet.dart +++ b/lib/src/parse/stylesheet.dart @@ -57,6 +57,10 @@ abstract class StylesheetParser extends Parser { /// Whether the parser is currently within a parenthesized expression. var _inParentheses = false; + /// The silent comment this parser encountered previously. + @protected + SilentComment lastSilentComment; + StylesheetParser(String contents, {url, Logger logger}) : super(contents, url: url, logger: logger); @@ -147,6 +151,8 @@ abstract class StylesheetParser extends Parser { /// Consumes a variable declaration. @protected VariableDeclaration variableDeclaration() { + var precedingComment = lastSilentComment; + lastSilentComment = null; var start = scanner.state; var name = variableName(); @@ -179,7 +185,7 @@ abstract class StylesheetParser extends Parser { expectStatementSeparator("variable declaration"); return VariableDeclaration(name, value, scanner.spanFrom(start), - guarded: guarded, global: global); + guarded: guarded, global: global, comment: precedingComment); } /// Consumes a style rule. @@ -681,6 +687,8 @@ abstract class StylesheetParser extends Parser { /// /// [start] should point before the `@`. FunctionRule _functionRule(LineScannerState start) { + var precedingComment = lastSilentComment; + lastSilentComment = null; var name = identifier(); whitespace(); var arguments = _argumentDeclaration(); @@ -708,7 +716,8 @@ abstract class StylesheetParser extends Parser { whitespace(); var children = this.children(_functionAtRule); - return FunctionRule(name, arguments, children, scanner.spanFrom(start)); + return FunctionRule(name, arguments, children, scanner.spanFrom(start), + comment: precedingComment); } /// Consumes a `@for` rule. @@ -937,6 +946,8 @@ abstract class StylesheetParser extends Parser { /// /// [start] should point before the `@`. MixinRule _mixinRule(LineScannerState start) { + var precedingComment = lastSilentComment; + lastSilentComment = null; var name = identifier(); whitespace(); var arguments = scanner.peekChar() == $lparen @@ -960,7 +971,7 @@ abstract class StylesheetParser extends Parser { _mixinHasContent = null; return MixinRule(name, arguments, children, scanner.spanFrom(start), - hasContent: hadContent); + hasContent: hadContent, comment: precedingComment); } /// Consumes a `@moz-document` rule. diff --git a/test/doc_comments_test.dart b/test/doc_comments_test.dart new file mode 100644 index 00000000..9abdbcb8 --- /dev/null +++ b/test/doc_comments_test.dart @@ -0,0 +1,200 @@ +// Copyright 2018 Google Inc. Use of this source code is governed by an +// MIT-style license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import 'package:sass/src/ast/sass.dart'; +import 'package:test/test.dart'; + +void main() { + group('documentation comments', () { + group('in SCSS', () { + test('attach to variable declarations', () { + final contents = r''' + /// Results my vary. + $vary: 5.16em;'''; + final stylesheet = Stylesheet.parseScss(contents); + final variable = + stylesheet.children.whereType().first; + + expect(variable.comment.docComment, equals('Results my vary.')); + }); + + test('attach to function rules', () { + final contents = r''' + /// A fun function! + @function fun($val) { + // Not a doc comment. + @return ($val / 1000) * 1em; + }'''; + final stylesheet = Stylesheet.parseScss(contents); + final function = stylesheet.children.whereType().first; + + expect(function.comment.docComment, equals('A fun function!')); + }); + + test('attach to mixin rules', () { + final contents = r''' + /// Mysterious mixin. + @mixin mystery { + // All black. + color: black; + background-color: black; + }'''; + final stylesheet = Stylesheet.parseScss(contents); + final mix = stylesheet.children.whereType().first; + + expect(mix.comment.docComment, equals('Mysterious mixin.')); + }); + + test('are null when there are no triple-slash comments', () { + final contents = r''' + // Regular comment. + $vary: 5.16em;'''; + final stylesheet = Stylesheet.parseScss(contents); + final variable = + stylesheet.children.whereType().first; + + expect(variable.comment.docComment, isNull); + }); + + test('are not carried over across members', () { + final contents = r''' + /// Mysterious mixin. + @mixin mystery { + // All black. + color: black; + background-color: black; + } + + /// A fun function! + @function fun($val) { + // Not a doc comment. + @return ($val / 1000) * 1em; + }'''; + final stylesheet = Stylesheet.parseScss(contents); + final mix = stylesheet.children.whereType().first; + final function = stylesheet.children.whereType().first; + + expect(mix.comment.docComment, equals('Mysterious mixin.')); + expect(function.comment.docComment, equals('A fun function!')); + }); + + test('do not include double-slash comments', () { + final contents = r''' + // Not a doc comment. + /// Line 1 + /// Line 2 + // Not a doc comment. + /// Line 3 + // Not a doc comment. + $vary: 5.16em;'''; + final stylesheet = Stylesheet.parseScss(contents); + final variable = + stylesheet.children.whereType().first; + + expect(variable.comment.docComment, equals('Line 1\nLine 2\nLine 3')); + }); + }); + + group('in indented syntax', () { + test('attach to variable declarations', () { + final contents = r''' +/// Results my vary. +$vary: 5.16em'''; + final stylesheet = Stylesheet.parseSass(contents); + final variable = + stylesheet.children.whereType().first; + + expect(variable.comment.docComment, equals('Results my vary.')); + }); + + test('attach to function rules', () { + final contents = r''' +/// A fun function! +@function fun($val) + // Not a doc comment. + @return ($val / 1000) * 1em'''; + final stylesheet = Stylesheet.parseSass(contents); + final function = stylesheet.children.whereType().first; + + expect(function.comment.docComment, equals('A fun function!')); + }); + + test('attach to mixin rules', () { + final contents = r''' +/// Mysterious mixin. +@mixin mystery + // All black. + color: black + background-color: black'''; + final stylesheet = Stylesheet.parseSass(contents); + final mix = stylesheet.children.whereType().first; + + expect(mix.comment.docComment, equals('Mysterious mixin.')); + }); + + test('are null when there are no triple-slash comments', () { + final contents = r''' +// Regular comment. +$vary: 5.16em'''; + final stylesheet = Stylesheet.parseSass(contents); + final variable = + stylesheet.children.whereType().first; + + expect(variable.comment.docComment, isNull); + }); + + test('are not carried over across members', () { + final contents = r''' +/// Mysterious mixin. +@mixin mystery + // All black. + color: black + background-color: black + +/// A fun function! +@function fun($val) + // Not a doc comment. + @return ($val / 1000) * 1em'''; + final stylesheet = Stylesheet.parseSass(contents); + final mix = stylesheet.children.whereType().first; + final function = stylesheet.children.whereType().first; + + expect(mix.comment.docComment, equals('Mysterious mixin.')); + expect(function.comment.docComment, equals('A fun function!')); + }); + + test('do not include double-slash comments', () { + final contents = r''' +// Not a doc comment. +/// Line 1 + Line 2 +// Not a doc comment. + Should be ignored. +$vary: 5.16em'''; + final stylesheet = Stylesheet.parseSass(contents); + final variable = + stylesheet.children.whereType().first; + + expect(variable.comment.docComment, equals('Line 1\nLine 2')); + }); + + test('are compacted into one from adjacent comments', () { + final contents = r''' +// Not a doc comment. +/// Line 1 +/// Line 2 + Line 3 +/// Line 4 +$vary: 5.16em'''; + final stylesheet = Stylesheet.parseSass(contents); + final variable = + stylesheet.children.whereType().first; + + expect(stylesheet.children.length, equals(2)); + expect(variable.comment.docComment, + equals('Line 1\nLine 2\nLine 3\nLine 4')); + }); + }); + }); +} From 23968148d0b8a696f06f172421ee5c0763fd7d97 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 20 Dec 2018 16:22:39 -0800 Subject: [PATCH 2/3] Properly use ImportResult.sourceMapUrl for source map URLs This also uses data: URLs to refer to stylesheets from stdin in source maps. --- CHANGELOG.md | 10 +++++++ lib/src/async_compile.dart | 14 +++++++++ lib/src/async_import_cache.dart | 25 ++++++++++++---- lib/src/compile.dart | 17 ++++++++++- lib/src/executable/compile_stylesheet.dart | 14 ++++----- lib/src/executable/options.dart | 4 +++ lib/src/import_cache.dart | 27 +++++++++++++---- lib/src/utils.dart | 7 +++++ test/cli/shared/source_maps.dart | 7 +++-- test/dart_api/importer_test.dart | 35 ++++++++++++++++++++++ 10 files changed, 137 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a814ac60..37b329e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ * Match Ruby Sass's behavior in some edge-cases involving numbers with many significant digits. +### Command-Line Interface + +* The source map generated for a stylesheet read from standard input now uses a + `data:` URL to include that stylesheet's contents in the source map. + +### Dart API + +* The URL used in a source map to refer to a stylesheet loaded from an importer + is now `ImportResult.sourceMapUrl` as documented. + ## 1.15.2 ### Node JS API diff --git a/lib/src/async_compile.dart b/lib/src/async_compile.dart index b300590e..9559a334 100644 --- a/lib/src/async_compile.dart +++ b/lib/src/async_compile.dart @@ -3,6 +3,7 @@ // https://opensource.org/licenses/MIT. import 'dart:async'; +import 'dart:convert'; import 'package:path/path.dart' as p; import 'package:source_maps/source_maps.dart'; @@ -17,6 +18,7 @@ import 'io.dart'; import 'logger.dart'; import 'sync_package_resolver.dart'; import 'syntax.dart'; +import 'utils.dart'; import 'visitor/async_evaluate.dart'; import 'visitor/serialize.dart'; @@ -130,6 +132,18 @@ Future _compileStylesheet( lineFeed: lineFeed, sourceMap: sourceMap); + if (serializeResult.sourceMap != null && importCache != null) { + // TODO(nweiz): Don't explicitly use a type parameter when dart-lang/sdk#25490 + // is fixed. + mapInPlace( + serializeResult.sourceMap.urls, + (url) => url == '' + ? Uri.dataFromString(stylesheet.span.file.getText(0), + encoding: utf8) + .toString() + : importCache.sourceMapUrl(Uri.parse(url)).toString()); + } + return CompileResult(evaluateResult, serializeResult); } diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart index d00966bf..4c8f6101 100644 --- a/lib/src/async_import_cache.dart +++ b/lib/src/async_import_cache.dart @@ -10,6 +10,7 @@ import 'package:tuple/tuple.dart'; import 'ast/sass.dart'; import 'importer.dart'; +import 'importer/result.dart'; import 'io.dart'; import 'logger.dart'; import 'sync_package_resolver.dart'; @@ -34,6 +35,9 @@ class AsyncImportCache { /// The parsed stylesheets for each canonicalized import URL. final Map _importCache; + /// The import results for each canonicalized import URL. + final Map _resultsCache; + /// Creates an import cache that resolves imports using [importers]. /// /// Imports are resolved by trying, in order: @@ -58,14 +62,16 @@ class AsyncImportCache { : _importers = _toImporters(importers, loadPaths, packageResolver), _logger = logger ?? const Logger.stderr(), _canonicalizeCache = {}, - _importCache = {}; + _importCache = {}, + _resultsCache = {}; /// Creates an import cache without any globally-avaiable importers. AsyncImportCache.none({Logger logger}) : _importers = const [], _logger = logger ?? const Logger.stderr(), _canonicalizeCache = {}, - _importCache = {}; + _importCache = {}, + _resultsCache = {}; /// Converts the user's [importers], [loadPaths], and [packageResolver] /// options into a single list of importers. @@ -96,7 +102,8 @@ class AsyncImportCache { : _importers = const [], _logger = const Logger.stderr(), _canonicalizeCache = const {}, - _importCache = const {}; + _importCache = const {}, + _resultsCache = const {}; /// Canonicalizes [url] according to one of this cache's importers. /// @@ -177,6 +184,8 @@ Relative canonical URLs are deprecated and will eventually be disallowed. return await putIfAbsentAsync(_importCache, canonicalUrl, () async { var result = await importer.load(canonicalUrl); if (result == null) return null; + + _resultsCache[canonicalUrl] = result; return Stylesheet.parse(result.contents, result.syntax, // For backwards-compatibility, relative canonical URLs are resolved // relative to [originalUrl]. @@ -189,8 +198,7 @@ Relative canonical URLs are deprecated and will eventually be disallowed. /// Return a human-friendly URL for [canonicalUrl] to use in a stack trace. /// - /// Throws a [StateError] if the stylesheet for [canonicalUrl] hasn't been - /// loaded by this cache. + /// Returns [canonicalUrl] as-is if it hasn't been loaded by this cache. Uri humanize(Uri canonicalUrl) { // Display the URL with the shortest path length. var url = minBy( @@ -206,6 +214,12 @@ Relative canonical URLs are deprecated and will eventually be disallowed. return url.resolve(p.url.basename(canonicalUrl.path)); } + /// Returns the URL to use in the source map to refer to [canonicalUrl]. + /// + /// Returns [canonicalUrl] as-is if it hasn't been loaded by this cache. + Uri sourceMapUrl(Uri canonicalUrl) => + _resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl; + /// Clears the cached canonical version of the given [url]. /// /// Has no effect if the canonical version of [url] has not been cached. @@ -218,6 +232,7 @@ Relative canonical URLs are deprecated and will eventually be disallowed. /// /// Has no effect if the imported file at [canonicalUrl] has not been cached. void clearImport(Uri canonicalUrl) { + _resultsCache.remove(canonicalUrl); _importCache.remove(canonicalUrl); } } diff --git a/lib/src/compile.dart b/lib/src/compile.dart index 872976b4..6d332461 100644 --- a/lib/src/compile.dart +++ b/lib/src/compile.dart @@ -5,13 +5,15 @@ // DO NOT EDIT. This file was generated from async_compile.dart. // See tool/synchronize.dart for details. // -// Checksum: e6de63e581c0f4da96756b9e2904bd81a090dc5b +// Checksum: 2bb00947655b3add16335253802a82188d730595 // // ignore_for_file: unused_import import 'async_compile.dart'; export 'async_compile.dart'; +import 'dart:convert'; + import 'package:path/path.dart' as p; import 'package:source_maps/source_maps.dart'; import 'package:source_span/source_span.dart'; @@ -25,6 +27,7 @@ import 'io.dart'; import 'logger.dart'; import 'sync_package_resolver.dart'; import 'syntax.dart'; +import 'utils.dart'; import 'visitor/evaluate.dart'; import 'visitor/serialize.dart'; @@ -138,5 +141,17 @@ CompileResult _compileStylesheet( lineFeed: lineFeed, sourceMap: sourceMap); + if (serializeResult.sourceMap != null && importCache != null) { + // TODO(nweiz): Don't explicitly use a type parameter when dart-lang/sdk#25490 + // is fixed. + mapInPlace( + serializeResult.sourceMap.urls, + (url) => url == '' + ? Uri.dataFromString(stylesheet.span.file.getText(0), + encoding: utf8) + .toString() + : importCache.sourceMapUrl(Uri.parse(url)).toString()); + } + return CompileResult(evaluateResult, serializeResult); } diff --git a/lib/src/executable/compile_stylesheet.dart b/lib/src/executable/compile_stylesheet.dart index c32c4816..5c82b097 100644 --- a/lib/src/executable/compile_stylesheet.dart +++ b/lib/src/executable/compile_stylesheet.dart @@ -15,6 +15,7 @@ import '../importer/filesystem.dart'; import '../io.dart'; import '../stylesheet_graph.dart'; import '../syntax.dart'; +import '../utils.dart'; import 'options.dart'; /// Compiles the stylesheet at [source] to [destination]. @@ -126,15 +127,10 @@ String _writeSourceMap( sourceMap.targetUrl = p.toUri(p.basename(destination)).toString(); } - for (var i = 0; i < sourceMap.urls.length; i++) { - var url = sourceMap.urls[i]; - - // The special URL "" indicates a file that came from stdin. - if (url == "") continue; - - sourceMap.urls[i] = - options.sourceMapUrl(Uri.parse(url), destination).toString(); - } + // TODO(nweiz): Don't explicitly use a type parameter when dart-lang/sdk#25490 + // is fixed. + mapInPlace(sourceMap.urls, + (url) => options.sourceMapUrl(Uri.parse(url), destination).toString()); var sourceMapText = jsonEncode(sourceMap.toJson(includeSourceContents: options.embedSources)); diff --git a/lib/src/executable/options.dart b/lib/src/executable/options.dart index b5e74f1b..41451d61 100644 --- a/lib/src/executable/options.dart +++ b/lib/src/executable/options.dart @@ -402,7 +402,11 @@ class ExecutableOptions { /// Makes [url] absolute or relative (to the directory containing /// [destination]) according to the `source-map-urls` option. + /// + /// If [url] isn't a `file:` URL, returns it as-is. Uri sourceMapUrl(Uri url, String destination) { + if (url.scheme.isNotEmpty && url.scheme != 'file') return url; + var path = p.canonicalize(p.fromUri(url)); return p.toUri(_options['source-map-urls'] == 'relative' ? p.relative(path, from: p.dirname(destination)) diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart index 34bc54ef..9fc7a464 100644 --- a/lib/src/import_cache.dart +++ b/lib/src/import_cache.dart @@ -5,7 +5,7 @@ // DO NOT EDIT. This file was generated from async_import_cache.dart. // See tool/synchronize.dart for details. // -// Checksum: 4989811a7b432e181ccc42004e91f4fe54a786ca +// Checksum: 291afac7e0d5edc6610685b28741786f667f83e8 // // ignore_for_file: unused_import @@ -15,6 +15,7 @@ import 'package:tuple/tuple.dart'; import 'ast/sass.dart'; import 'importer.dart'; +import 'importer/result.dart'; import 'io.dart'; import 'logger.dart'; import 'sync_package_resolver.dart'; @@ -39,6 +40,9 @@ class ImportCache { /// The parsed stylesheets for each canonicalized import URL. final Map _importCache; + /// The import results for each canonicalized import URL. + final Map _resultsCache; + /// Creates an import cache that resolves imports using [importers]. /// /// Imports are resolved by trying, in order: @@ -63,14 +67,16 @@ class ImportCache { : _importers = _toImporters(importers, loadPaths, packageResolver), _logger = logger ?? const Logger.stderr(), _canonicalizeCache = {}, - _importCache = {}; + _importCache = {}, + _resultsCache = {}; /// Creates an import cache without any globally-avaiable importers. ImportCache.none({Logger logger}) : _importers = const [], _logger = logger ?? const Logger.stderr(), _canonicalizeCache = {}, - _importCache = {}; + _importCache = {}, + _resultsCache = {}; /// Converts the user's [importers], [loadPaths], and [packageResolver] /// options into a single list of importers. @@ -101,7 +107,8 @@ class ImportCache { : _importers = const [], _logger = const Logger.stderr(), _canonicalizeCache = const {}, - _importCache = const {}; + _importCache = const {}, + _resultsCache = const {}; /// Canonicalizes [url] according to one of this cache's importers. /// @@ -181,6 +188,8 @@ Relative canonical URLs are deprecated and will eventually be disallowed. return _importCache.putIfAbsent(canonicalUrl, () { var result = importer.load(canonicalUrl); if (result == null) return null; + + _resultsCache[canonicalUrl] = result; return Stylesheet.parse(result.contents, result.syntax, // For backwards-compatibility, relative canonical URLs are resolved // relative to [originalUrl]. @@ -193,8 +202,7 @@ Relative canonical URLs are deprecated and will eventually be disallowed. /// Return a human-friendly URL for [canonicalUrl] to use in a stack trace. /// - /// Throws a [StateError] if the stylesheet for [canonicalUrl] hasn't been - /// loaded by this cache. + /// Returns [canonicalUrl] as-is if it hasn't been loaded by this cache. Uri humanize(Uri canonicalUrl) { // Display the URL with the shortest path length. var url = minBy( @@ -210,6 +218,12 @@ Relative canonical URLs are deprecated and will eventually be disallowed. return url.resolve(p.url.basename(canonicalUrl.path)); } + /// Returns the URL to use in the source map to refer to [canonicalUrl]. + /// + /// Returns [canonicalUrl] as-is if it hasn't been loaded by this cache. + Uri sourceMapUrl(Uri canonicalUrl) => + _resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl; + /// Clears the cached canonical version of the given [url]. /// /// Has no effect if the canonical version of [url] has not been cached. @@ -222,6 +236,7 @@ Relative canonical URLs are deprecated and will eventually be disallowed. /// /// Has no effect if the imported file at [canonicalUrl] has not been cached. void clearImport(Uri canonicalUrl) { + _resultsCache.remove(canonicalUrl); _importCache.remove(canonicalUrl); } } diff --git a/lib/src/utils.dart b/lib/src/utils.dart index bbc540c0..e3a1b8fb 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -289,6 +289,13 @@ Map normalizedMapMap(Map map, return result; } +/// Destructively updates every element of [list] with the result of [function]. +void mapInPlace(List list, T function(T element)) { + for (var i = 0; i < list.length; i++) { + list[i] = function(list[i]); + } +} + /// Returns the longest common subsequence between [list1] and [list2]. /// /// If there are more than one equally long common subsequence, returns the one diff --git a/test/cli/shared/source_maps.dart b/test/cli/shared/source_maps.dart index fb05ebe5..94915160 100644 --- a/test/cli/shared/source_maps.dart +++ b/test/cli/shared/source_maps.dart @@ -116,13 +116,16 @@ void sharedTests(Future runSass(Iterable arguments)) { }); }); - test("with --stdin uses an empty string", () async { + test("with --stdin uses a data: URL", () async { var sass = await runSass(["--stdin", "out.css"]); sass.stdin.writeln("a {b: c}"); sass.stdin.close(); await sass.shouldExit(0); - expect(_readJson("out.css.map"), containsPair("sources", [""])); + expect( + _readJson("out.css.map"), + containsPair("sources", + [Uri.dataFromString("a {b: c}\n", encoding: utf8).toString()])); }); group("with --no-source-map,", () { diff --git a/test/dart_api/importer_test.dart b/test/dart_api/importer_test.dart index 1c76a934..34f71883 100644 --- a/test/dart_api/importer_test.dart +++ b/test/dart_api/importer_test.dart @@ -4,6 +4,9 @@ @TestOn('vm') +import 'dart:convert'; + +import 'package:source_maps/source_maps.dart'; import 'package:test/test.dart'; import 'package:sass/sass.dart'; @@ -86,6 +89,38 @@ main() { ''')); }); + test("uses an importer's source map URL", () { + SingleMapping map; + compileString('@import "orange";', + importers: [ + _TestImporter((url) => Uri.parse("u:$url"), (url) { + var color = url.path; + return ImporterResult('.$color {color: $color}', + sourceMapUrl: Uri.parse("u:blue"), indented: false); + }) + ], + sourceMap: (map_) => map = map_); + + expect(map.urls, contains("u:blue")); + }); + + test("uses a data: source map URL if the importer doesn't provide one", () { + SingleMapping map; + compileString('@import "orange";', + importers: [ + _TestImporter((url) => Uri.parse("u:$url"), (url) { + var color = url.path; + return ImporterResult('.$color {color: $color}', indented: false); + }) + ], + sourceMap: (map_) => map = map_); + + expect( + map.urls, + contains(Uri.dataFromString(".orange {color: orange}", encoding: utf8) + .toString())); + }); + test("wraps an error in canonicalize()", () { expect(() { compileString('@import "orange";', importers: [ From 7ae11f1c0e60d16d6e118dcc4f489f8a90eaa3c8 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 20 Dec 2018 15:03:28 -0800 Subject: [PATCH 3/3] Make synchronize_test draw from the canonical list of synced files --- test/synchronize_test.dart | 9 +++------ tool/grind/synchronize.dart | 4 ++-- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/test/synchronize_test.dart b/test/synchronize_test.dart index c62b40b2..8684e817 100644 --- a/test/synchronize_test.dart +++ b/test/synchronize_test.dart @@ -10,14 +10,11 @@ import 'dart:io'; import 'package:crypto/crypto.dart'; import 'package:test/test.dart'; +import '../tool/grind/synchronize.dart' as synchronize; + void main() { test("synchronized files are up-to-date", () { - ({ - 'lib/src/visitor/async_evaluate.dart': 'lib/src/visitor/evaluate.dart', - 'lib/src/async_environment.dart': 'lib/src/environment.dart', - 'lib/src/async_import_cache.dart': 'lib/src/import_cache.dart' - }) - .forEach((sourcePath, targetPath) { + synchronize.sources.forEach((sourcePath, targetPath) { var source = File(sourcePath).readAsStringSync(); var target = File(targetPath).readAsStringSync(); diff --git a/tool/grind/synchronize.dart b/tool/grind/synchronize.dart index ed7583a5..6903f821 100644 --- a/tool/grind/synchronize.dart +++ b/tool/grind/synchronize.dart @@ -13,7 +13,7 @@ import 'package:grinder/grinder.dart'; import 'package:path/path.dart' as p; /// The files to compile to synchronous versions. -final _sources = const { +final sources = const { 'lib/src/visitor/async_evaluate.dart': 'lib/src/visitor/evaluate.dart', 'lib/src/async_compile.dart': 'lib/src/compile.dart', 'lib/src/async_environment.dart': 'lib/src/environment.dart', @@ -38,7 +38,7 @@ final _sharedClasses = const ['EvaluateResult', 'CompileResult']; /// to a synchronous equivalent. @Task('Compile async code to synchronous code.') synchronize() { - _sources.forEach((source, target) { + sources.forEach((source, target) { var visitor = _Visitor(File(source).readAsStringSync(), source); parseDartFile(source).accept(visitor); var formatted = DartFormatter().format(visitor.result);