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.

Sign up for GitHub

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

Jump to bottom

Add a --pkg-importer flag #2169

Merged
merged 2 commits into from
Feb 12, 2024
Merged

Add a --pkg-importer flag #2169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e26681e
Add a `--pkg-importer` flag
nex3 Feb 7, 2024
12f397f
Make --pkg-importer take a list of options
nex3 Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ announcement][pkg-importers] on the Sass blog.

[pkg-importers]: https://sass-lang.com/blog/announcing-pkg-importers

### Command-Line Interface

* Add a `--pkg-importer` flag to enable built-in `pkg:` importers. Currently
this only supports the Node.js package resolution algorithm, via
`--pkg-importer=node`. For example, `@use "pkg:bootstrap"` will load
`node_modules/bootstrap/scss/bootstrap.scss`.

### JavaScript API

* Add a `NodePackageImporter` importer that can be passed to the `importers`
Expand Down
1 change: 1 addition & 0 deletions bin/sass.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Future<void> main(List<String> args) async {
}

var graph = StylesheetGraph(ImportCache(
importers: options.pkgImporters,
loadPaths: options.loadPaths,
// This logger is only used for handling fatal/future deprecations
// during parsing, and is re-used across parses, so we don't want to
Expand Down
4 changes: 3 additions & 1 deletion lib/src/executable/compile_stylesheet.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ Future<void> _compileStylesheetWithoutErrorHandling(ExecutableOptions options,
try {
if (options.asynchronous) {
var importCache = AsyncImportCache(
loadPaths: options.loadPaths, logger: options.logger);
importers: options.pkgImporters,
loadPaths: options.loadPaths,
logger: options.logger);

result = source == null
? await compileStringAsync(await readStdin(),
Expand Down
13 changes: 13 additions & 0 deletions lib/src/executable/options.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import 'package:pub_semver/pub_semver.dart';
import 'package:term_glyph/term_glyph.dart' as term_glyph;

import '../../sass.dart';
import '../importer/node_package.dart';
import '../io.dart';
import '../util/character.dart';

Expand Down Expand Up @@ -47,6 +48,12 @@ final class ExecutableOptions {
help: 'A path to use when resolving imports.\n'
'May be passed multiple times.',
splitCommas: false)
..addMultiOption('pkg-importer',
abbr: 'p',
valueHelp: 'TYPE',
allowed: ['node'],
help: 'Built-in importer(s) to use for pkg: URLs.',
allowedHelp: {'node': 'Load files like Node.js package resolution.'})
..addOption('style',
abbr: 's',
valueHelp: 'NAME',
Expand Down Expand Up @@ -218,6 +225,12 @@ final class ExecutableOptions {
/// The set of paths Sass in which should look for imported files.
List<String> get loadPaths => _options['load-path'] as List<String>;

/// The list of built-in importers to use to load `pkg:` URLs.
List<Importer> get pkgImporters => [
for (var _ in _options['pkg-importer'] as List<String>)
NodePackageImporter('.')
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better if the entry point dir is customizable. E.g. --node-pkg-importer=/entrypoint/dir and maybe implicitly use pwd if not directory is not provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would only ever be relevant when resolving a pkg: importer from stdin, at which point I think it's easy enough to just change the CWD of the executable if it really matters. Adding an additional flag for it is pretty heavyweight, so I'd like to avoid it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe clarify that this is PWD based in the help?

Copy link
Contributor

@ntkme ntkme Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My concern is that tomorrow if we have another --pkg-importer=x and user may want to have both --pkg-importer=node and --pkg-importer=x. In theory it's fine to have more than one pkg importer, that they should be attempted in order just like other importers. While it is ok to repeat the --pkg-importer option, but at the same time, it is a bit confusing whether it means overriding previous option or repeating the same option with different parameters, that's why I thought --node-pkg-importer might be better than --pkg-importer=node.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe this should be a list option e.g. --pkg-importer=node,x,y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was intended to be a list option. Having it that way, rather than separate named options, allows users to explicitly specify the order of multiple built-in pkg: importers, which I think is important.

];

/// Whether to run the evaluator in asynchronous mode, for debugging purposes.
bool get asynchronous => _options['async'] as bool;

Expand Down
5 changes: 4 additions & 1 deletion lib/src/executable/repl.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ Future<void> repl(ExecutableOptions options) async {
var logger = TrackingLogger(options.logger);
var evaluator = Evaluator(
importer: FilesystemImporter.cwd,
importCache: ImportCache(loadPaths: options.loadPaths, logger: logger),
importCache: ImportCache(
importers: options.pkgImporters,
loadPaths: options.loadPaths,
logger: logger),
logger: logger);
await for (String line in repl.runAsync()) {
if (line.trim().isEmpty) continue;
Expand Down
4 changes: 4 additions & 0 deletions pkg/sass_api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 9.4.0

* No user-visible changes.

## 9.3.0

* No user-visible changes.
Expand Down
4 changes: 2 additions & 2 deletions pkg/sass_api/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ name: sass_api
# Note: Every time we add a new Sass AST node, we need to bump the *major*
# version because it's a breaking change for anyone who's implementing the
# visitor interface(s).
version: 9.3.0
version: 9.4.0
description: Additional APIs for Dart Sass.
homepage: https://github.com/sass/dart-sass

environment:
sdk: ">=3.0.0 <4.0.0"

dependencies:
sass: 1.70.0
sass: 1.71.0

dev_dependencies:
dartdoc: ^6.0.0
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: sass
version: 1.71.0-dev
version: 1.71.0
description: A Sass implementation in Dart.
homepage: https://github.com/sass/dart-sass

Expand Down
Loading