Skip to content
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

Fix import-only files for Node importers #919

Merged
merged 2 commits into from Jan 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
@@ -1,3 +1,8 @@
## 1.24.2

* Fix a bug introduced in the previous release that prevented custom importers
in Node.js from loading import-only files.

## 1.24.1

* Fix a bug where the wrong file could be loaded when the same URL is used by
Expand Down
53 changes: 32 additions & 21 deletions lib/src/importer/node/implementation.dart
Expand Up @@ -67,10 +67,10 @@ class NodeImporter {
/// The [previous] URL is the URL of the stylesheet in which the import
/// appeared. Returns the contents of the stylesheet and the URL to use as
/// [previous] for imports within the loaded stylesheet.
Tuple2<String, String> load(String url, Uri previous) {
Tuple2<String, String> load(String url, Uri previous, bool forImport) {
var parsed = Uri.parse(url);
if (parsed.scheme == '' || parsed.scheme == 'file') {
var result = _resolveRelativePath(p.fromUri(parsed), previous);
var result = _resolveRelativePath(p.fromUri(parsed), previous, forImport);
if (result != null) return result;
}

Expand All @@ -79,21 +79,24 @@ class NodeImporter {
previous.scheme == 'file' ? p.fromUri(previous) : previous.toString();
for (var importer in _importers) {
var value = call2(importer, _context, url, previousString);
if (value != null) return _handleImportResult(url, previous, value);
if (value != null) {
return _handleImportResult(url, previous, value, forImport);
}
}

return _resolveLoadPathFromUrl(parsed, previous);
return _resolveLoadPathFromUrl(parsed, previous, forImport);
}

/// Asynchronously loads the stylesheet at [url].
///
/// The [previous] URL is the URL of the stylesheet in which the import
/// appeared. Returns the contents of the stylesheet and the URL to use as
/// [previous] for imports within the loaded stylesheet.
Future<Tuple2<String, String>> loadAsync(String url, Uri previous) async {
Future<Tuple2<String, String>> loadAsync(
String url, Uri previous, bool forImport) async {
var parsed = Uri.parse(url);
if (parsed.scheme == '' || parsed.scheme == 'file') {
var result = _resolveRelativePath(p.fromUri(parsed), previous);
var result = _resolveRelativePath(p.fromUri(parsed), previous, forImport);
if (result != null) return result;
}

Expand All @@ -102,22 +105,26 @@ class NodeImporter {
previous.scheme == 'file' ? p.fromUri(previous) : previous.toString();
for (var importer in _importers) {
var value = await _callImporterAsync(importer, url, previousString);
if (value != null) return _handleImportResult(url, previous, value);
if (value != null) {
return _handleImportResult(url, previous, value, forImport);
}
}

return _resolveLoadPathFromUrl(parsed, previous);
return _resolveLoadPathFromUrl(parsed, previous, forImport);
}

/// Tries to load a stylesheet at the given [path] relative to [previous].
///
/// Returns the stylesheet at that path and the URL used to load it, or `null`
/// if loading failed.
Tuple2<String, String> _resolveRelativePath(String path, Uri previous) {
if (p.isAbsolute(path)) return _tryPath(path);
Tuple2<String, String> _resolveRelativePath(
String path, Uri previous, bool forImport) {
if (p.isAbsolute(path)) return _tryPath(path, forImport);

// 1: Filesystem imports relative to the base file.
if (previous.scheme == 'file') {
var result = _tryPath(p.join(p.dirname(p.fromUri(previous)), path));
var result =
_tryPath(p.join(p.dirname(p.fromUri(previous)), path), forImport);
if (result != null) return result;
}
return null;
Expand All @@ -128,24 +135,26 @@ class NodeImporter {
///
/// Returns the stylesheet at that path and the URL used to load it, or `null`
/// if loading failed.
Tuple2<String, String> _resolveLoadPathFromUrl(Uri url, Uri previous) =>
Tuple2<String, String> _resolveLoadPathFromUrl(
Uri url, Uri previous, bool forImport) =>
url.scheme == '' || url.scheme == 'file'
? _resolveLoadPath(p.fromUri(url), previous)
? _resolveLoadPath(p.fromUri(url), previous, forImport)
: null;

/// Tries to load a stylesheet at the given [path] from a load path (including
/// the working directory).
///
/// Returns the stylesheet at that path and the URL used to load it, or `null`
/// if loading failed.
Tuple2<String, String> _resolveLoadPath(String path, Uri previous) {
Tuple2<String, String> _resolveLoadPath(
String path, Uri previous, bool forImport) {
// 2: Filesystem imports relative to the working directory.
var cwdResult = _tryPath(p.absolute(path));
var cwdResult = _tryPath(p.absolute(path), forImport);
if (cwdResult != null) return cwdResult;

// 3: Filesystem imports relative to [_includePaths].
for (var includePath in _includePaths) {
var result = _tryPath(p.absolute(p.join(includePath, path)));
var result = _tryPath(p.absolute(p.join(includePath, path)), forImport);
if (result != null) return result;
}

Expand All @@ -156,8 +165,10 @@ class NodeImporter {
///
/// Returns the stylesheet at that path and the URL used to load it, or `null`
/// if loading failed.
Tuple2<String, String> _tryPath(String path) {
var resolved = resolveImportPath(path);
Tuple2<String, String> _tryPath(String path, bool forImport) {
var resolved = forImport
? inImportRule(() => resolveImportPath(path))
: resolveImportPath(path);
return resolved == null
? null
: Tuple2(readFile(resolved), p.toUri(resolved).toString());
Expand All @@ -166,14 +177,14 @@ class NodeImporter {
/// Converts an importer's return [value] to a tuple that can be returned by
/// [load].
Tuple2<String, String> _handleImportResult(
String url, Uri previous, Object value) {
String url, Uri previous, Object value, bool forImport) {
if (isJSError(value)) throw value;
if (value is! NodeImporterResult) return null;

var result = value as NodeImporterResult;
if (result.file != null) {
var resolved = _resolveRelativePath(result.file, previous) ??
_resolveLoadPath(result.file, previous);
var resolved = _resolveRelativePath(result.file, previous, forImport) ??
_resolveLoadPath(result.file, previous, forImport);
if (resolved != null) return resolved;

throw "Can't find stylesheet to import.";
Expand Down
6 changes: 4 additions & 2 deletions lib/src/importer/node/interface.dart
Expand Up @@ -10,7 +10,9 @@ class NodeImporter {
NodeImporter(Object context, Iterable<String> includePaths,
Iterable<Object> importers);

Tuple2<String, String> load(String url, Uri previous) => null;
Tuple2<String, String> load(String url, Uri previous, bool forImport) => null;

Future<Tuple2<String, String>> loadAsync(String url, Uri previous) => null;
Future<Tuple2<String, String>> loadAsync(
String url, Uri previous, bool forImport) =>
null;
}
8 changes: 4 additions & 4 deletions lib/src/visitor/async_evaluate.dart
Expand Up @@ -1410,7 +1410,7 @@ class _EvaluateVisitor
_importSpan = span;

if (_nodeImporter != null) {
var stylesheet = await _importLikeNode(url);
var stylesheet = await _importLikeNode(url, forImport);
if (stylesheet != null) return Tuple2(null, stylesheet);
} else {
var tuple = await _importCache.import(Uri.parse(url),
Expand Down Expand Up @@ -1445,9 +1445,9 @@ class _EvaluateVisitor
/// Imports a stylesheet using [_nodeImporter].
///
/// Returns the [Stylesheet], or `null` if the import failed.
Future<Stylesheet> _importLikeNode(String originalUrl) async {
var result =
await _nodeImporter.loadAsync(originalUrl, _stylesheet.span?.sourceUrl);
Future<Stylesheet> _importLikeNode(String originalUrl, bool forImport) async {
var result = await _nodeImporter.loadAsync(
originalUrl, _stylesheet.span?.sourceUrl, forImport);
if (result == null) return null;

var contents = result.item1;
Expand Down
9 changes: 5 additions & 4 deletions lib/src/visitor/evaluate.dart
Expand Up @@ -5,7 +5,7 @@
// DO NOT EDIT. This file was generated from async_evaluate.dart.
// See tool/grind/synchronize.dart for details.
//
// Checksum: e3d7bea705bed417db9925546c076caed4465b2e
// Checksum: 98bd4418d21d4f005485e343869b458b44841450
//
// ignore_for_file: unused_import

Expand Down Expand Up @@ -1407,7 +1407,7 @@ class _EvaluateVisitor
_importSpan = span;

if (_nodeImporter != null) {
var stylesheet = _importLikeNode(url);
var stylesheet = _importLikeNode(url, forImport);
if (stylesheet != null) return Tuple2(null, stylesheet);
} else {
var tuple = _importCache.import(Uri.parse(url),
Expand Down Expand Up @@ -1442,8 +1442,9 @@ class _EvaluateVisitor
/// Imports a stylesheet using [_nodeImporter].
///
/// Returns the [Stylesheet], or `null` if the import failed.
Stylesheet _importLikeNode(String originalUrl) {
var result = _nodeImporter.load(originalUrl, _stylesheet.span?.sourceUrl);
Stylesheet _importLikeNode(String originalUrl, bool forImport) {
var result =
_nodeImporter.load(originalUrl, _stylesheet.span?.sourceUrl, forImport);
if (result == null) return null;

var contents = result.item1;
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
@@ -1,5 +1,5 @@
name: sass
version: 1.24.1
version: 1.24.2
description: A Sass implementation in Dart.
author: Sass Team
homepage: https://github.com/sass/dart-sass
Expand Down
26 changes: 26 additions & 0 deletions test/node_api/importer_test.dart
Expand Up @@ -165,6 +165,32 @@ void main() {
equalsIgnoringWhitespace('a { b: c; }'));
});

test("supports import-only files", () async {
await writeTextFile(p.join(sandbox, 'target.scss'), 'a {b: regular}');
await writeTextFile(
p.join(sandbox, 'target.import.scss'), 'a {b: import-only}');

expect(
renderSync(RenderOptions(
data: "@import 'foo'",
importer: allowInterop((void _, void __) =>
NodeImporterResult(file: p.join(sandbox, 'target.scss'))))),
equalsIgnoringWhitespace('a { b: import-only; }'));
});

test("supports mixed `@use` and `@import`", () async {
await writeTextFile(p.join(sandbox, 'target.scss'), 'a {b: regular}');
await writeTextFile(
p.join(sandbox, 'target.import.scss'), 'a {b: import-only}');

expect(
renderSync(RenderOptions(
data: "@use 'foo'; @import 'foo';",
importer: allowInterop((void _, void __) =>
NodeImporterResult(file: p.join(sandbox, 'target.scss'))))),
equalsIgnoringWhitespace('a { b: regular; } a { b: import-only; }'));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also add a test to the top-level node_api_test that the default import logic supports both of these cases.


test("may be extensionless", () async {
expect(
renderSync(RenderOptions(
Expand Down
20 changes: 20 additions & 0 deletions test/node_api_test.dart
Expand Up @@ -77,6 +77,26 @@ void main() {
equalsIgnoringWhitespace('a { b: c; }'));
});

test("supports import-only files", () async {
await writeTextFile(p.join(sandbox, 'foo.scss'), 'a {b: regular}');
await writeTextFile(
p.join(sandbox, 'foo.import.scss'), 'a {b: import-only}');

runTestInSandbox();
expect(renderSync(RenderOptions(data: "@import 'foo'")),
equalsIgnoringWhitespace('a { b: import-only; }'));
});

test("supports mixed `@use` and `@import`", () async {
await writeTextFile(p.join(sandbox, 'foo.scss'), 'a {b: regular}');
await writeTextFile(
p.join(sandbox, 'foo.import.scss'), 'a {b: import-only}');

runTestInSandbox();
expect(renderSync(RenderOptions(data: "@use 'foo'; @import 'foo';")),
equalsIgnoringWhitespace('a { b: regular; } a { b: import-only; }'));
});

test("renders a string", () {
expect(renderSync(RenderOptions(data: "a {b: c}")),
equalsIgnoringWhitespace('a { b: c; }'));
Expand Down