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 --fatal-deprecations and --future-deprecations #1820

Merged
merged 15 commits into from
Mar 10, 2023
Merged

Add --fatal-deprecations and --future-deprecations #1820

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9a40e3a
Add fatal/futureDeprecations to Dart API
jathak Oct 27, 2022
8f3dae1
Add future deprecation for `@import`
jathak Oct 27, 2022
323aa1c
Add flags
jathak Oct 28, 2022
e3f1e9e
Merge colorUnits and randomWithUnits
jathak Oct 28, 2022
96df02f
Update changelogs and pubspecs
jathak Oct 28, 2022
d1f28f5
Merge remote-tracking branch 'origin/main' into deprecations
jathak Dec 6, 2022
7e23ec8
Merge remote-tracking branch 'origin/main' into deprecations
jathak Mar 3, 2023
6b3e6f0
Add tests
jathak Mar 4, 2023
d00498c
Merge remote-tracking branch 'origin/main' into deprecations
jathak Mar 4, 2023
a4bc07d
Code review
jathak Mar 6, 2023
a3a8974
Use isFuture instead of deprecatedIn == null
jathak Mar 6, 2023
1d38e63
More code review
jathak Mar 6, 2023
b03ac1d
Merge remote-tracking branch 'origin/main' into deprecations
jathak Mar 9, 2023
91c23dd
Split warnForDeprecation from warn
jathak Mar 9, 2023
2b99fef
Add missing word to error message
jathak Mar 9, 2023
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
31 changes: 31 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,34 @@
## 1.59.0

### Command Line Interface

* New `--fatal-deprecation` flag that lets you treat a deprecation warning as
jathak marked this conversation as resolved.
Show resolved Hide resolved
an error. You can pass an individual deprecation ID (e.g. `slash-div`) or you
can pass a Dart Sass version to treat all deprecations initially emitted in
that version or earlier as errors.

* New `--future-deprecation` flag that lets you opt into warning for use of
certain features that will be deprecated in the future. At the moment, the
only option is `--future-deprecation=import`, which will emit warnings for
Sass `@import` rules, which are not yet deprecated, but will be in the future.

### Dart API

* New `Deprecation` enum, which contains the different current and future
deprecations used by the new CLI flags.

* The optional `deprecation` boolean parameter of the `warn` function is now
deprecated. Use `deprecationType` to pass the specific `Deprecation` being
warned for instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if users are writing custom functions that have behavior that becomes deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking they would pass Deprecation.userAuthored, but do you think it would make more sense to just leave the boolean parameter un-deprecated? (since right now if they pass deprecation: true with no deprecationType, it defaults to Deprecation.userAuthored)

Or could it be confusing to have both deprecation and deprecationType un-deprecated, since then you could potentially call it with a deprecation type but explicitly pass deprecation: false? (in which case the boolean parameter would be ignored)

Would making deprecation nullable but un-deprecated work?

If we do leave the deprecation un-deprecated in EvaluationContext.warn, should we also do the same for Logger.warn? If we do that, should we just leave the warnForDeprecation extension for Logger as private? (since custom functions should usually be just using Deprecation.userAuthored anyway)

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we just have two separate functions for both EvaluationContext and Logger, one for public consumption that takes a boolean deprecation flag and one that's private to Sass which takes a Deprecation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


* Calling `Logger.warn` with the `deprecation` parameter is similarly
deprecated. To ensure that a deprecation warning can be properly handled by
the new flags, deprecation warnings should use the new `warnForDeprecation`
extension method on `Logger` instead.

* The `compile` methods now take in `fatalDeprecations` and `futureDeprecations`
parameters, which work similarly to the CLI flags.

## 1.58.4

* Pull `@font-face` to the root rather than bubbling the style rule selector
Expand Down
12 changes: 10 additions & 2 deletions bin/sass.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import 'package:sass/src/executable/repl.dart';
import 'package:sass/src/executable/watch.dart';
import 'package:sass/src/import_cache.dart';
import 'package:sass/src/io.dart';
import 'package:sass/src/logger/deprecation_handling.dart';
import 'package:sass/src/stylesheet_graph.dart';
import 'package:sass/src/utils.dart';

Expand Down Expand Up @@ -52,8 +53,15 @@ Future<void> main(List<String> args) async {
return;
}

var graph = StylesheetGraph(
ImportCache(loadPaths: options.loadPaths, logger: options.logger));
var graph = StylesheetGraph(ImportCache(
loadPaths: options.loadPaths,
logger: DeprecationHandlingLogger(options.logger,
fatalDeprecations: options.fatalDeprecations,
futureDeprecations: options.futureDeprecations,
// This logger may be reused by multiple parses, so we don't want
// warnings from a previous parse to cause subsequent warnings to
// be skipped.
limitRepetition: false)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a couple questions here:

  • Previously, the Options class was responsible for setting up the logger according to the options passed. Why not do the same with DeprecationHandlingLogger?

  • If we don't limit repetitions here, under what circumstances will we limit repetitions on the CLI?

Copy link
Member Author

Choose a reason for hiding this comment

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

This wrapped logger is only used during parsing. options.logger is wrapped by a DeprecationHandlingLogger again during the actual compilation (this time, with limitRepetition set to true if verbose is false).

This matches the current behavior with the TerseLogger that DeprecationHandlingLogger replaces (repetitions aren't limited for deprecation warnings during parsing). The need to reset the state of DeprecationHandlingLogger is also why we don't just have options.logger produce it.

We could change options.logger to just produce a single DeprecationHandlingLogger, but we'd then need some way for StylesheetGraph and the compiler to reset its state after each run. Do you think that makes more sense instead of wrapping the base logger multiple times?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I think this is fine. It might be worth documenting more of what you just said in the comments, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

if (options.watch) {
await watch(options, graph);
return;
Expand Down
30 changes: 23 additions & 7 deletions lib/sass.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'src/async_import_cache.dart';
import 'src/callable.dart';
import 'src/compile.dart' as c;
import 'src/compile_result.dart';
import 'src/deprecation.dart';
import 'src/exception.dart';
import 'src/import_cache.dart';
import 'src/importer.dart';
Expand All @@ -24,6 +25,7 @@ import 'src/visitor/serialize.dart';

export 'src/callable.dart' show Callable, AsyncCallable;
export 'src/compile_result.dart';
export 'src/deprecation.dart';
export 'src/exception.dart' show SassException;
export 'src/importer.dart';
export 'src/logger.dart';
Expand Down Expand Up @@ -105,7 +107,9 @@ CompileResult compileToResult(String path,
bool quietDeps = false,
bool verbose = false,
bool sourceMap = false,
bool charset = true}) =>
bool charset = true,
Set<Deprecation> fatalDeprecations = const {},
Set<Deprecation> futureDeprecations = const {}}) =>
jathak marked this conversation as resolved.
Show resolved Hide resolved
c.compile(path,
logger: logger,
importCache: ImportCache(
Expand All @@ -118,7 +122,9 @@ CompileResult compileToResult(String path,
quietDeps: quietDeps,
verbose: verbose,
sourceMap: sourceMap,
charset: charset);
charset: charset,
fatalDeprecations: fatalDeprecations,
futureDeprecations: futureDeprecations);

/// Compiles [source] to CSS and returns a [CompileResult] containing the CSS
/// and additional metadata about the compilation..
Expand Down Expand Up @@ -200,7 +206,9 @@ CompileResult compileStringToResult(String source,
bool quietDeps = false,
bool verbose = false,
bool sourceMap = false,
bool charset = true}) =>
bool charset = true,
Set<Deprecation> fatalDeprecations = const {},
Set<Deprecation> futureDeprecations = const {}}) =>
c.compileString(source,
syntax: syntax,
logger: logger,
Expand All @@ -216,7 +224,9 @@ CompileResult compileStringToResult(String source,
quietDeps: quietDeps,
verbose: verbose,
sourceMap: sourceMap,
charset: charset);
charset: charset,
fatalDeprecations: fatalDeprecations,
futureDeprecations: futureDeprecations);

/// Like [compileToResult], except it runs asynchronously.
///
Expand All @@ -234,7 +244,9 @@ Future<CompileResult> compileToResultAsync(String path,
bool quietDeps = false,
bool verbose = false,
bool sourceMap = false,
bool charset = true}) =>
bool charset = true,
Set<Deprecation> fatalDeprecations = const {},
Set<Deprecation> futureDeprecations = const {}}) =>
c.compileAsync(path,
logger: logger,
importCache: AsyncImportCache(
Expand All @@ -247,7 +259,9 @@ Future<CompileResult> compileToResultAsync(String path,
quietDeps: quietDeps,
verbose: verbose,
sourceMap: sourceMap,
charset: charset);
charset: charset,
fatalDeprecations: fatalDeprecations,
futureDeprecations: futureDeprecations);

/// Like [compileStringToResult], except it runs asynchronously.
///
Expand All @@ -270,7 +284,9 @@ Future<CompileResult> compileStringToResultAsync(String source,
bool quietDeps = false,
bool verbose = false,
bool sourceMap = false,
bool charset = true}) =>
bool charset = true,
Set<Deprecation> fatalDeprecations = const {},
Set<Deprecation> futureDeprecations = const {}}) =>
c.compileStringAsync(source,
syntax: syntax,
logger: logger,
Expand Down
3 changes: 2 additions & 1 deletion lib/src/ast/selector.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'package:meta/meta.dart';

import '../deprecation.dart';
import '../evaluation_context.dart';
import '../exception.dart';
import '../visitor/any_selector.dart';
Expand Down Expand Up @@ -88,7 +89,7 @@ abstract class Selector {
'This will be an error in Dart Sass 2.0.0.\n'
'\n'
'More info: https://sass-lang.com/d/bogus-combinators',
deprecation: true);
deprecationType: Deprecation.bogusCombinators);
}

/// Calls the appropriate visit method on [visitor].
Expand Down
29 changes: 20 additions & 9 deletions lib/src/async_compile.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ import 'ast/sass.dart';
import 'async_import_cache.dart';
import 'callable.dart';
import 'compile_result.dart';
import 'deprecation.dart';
import 'importer.dart';
import 'importer/legacy_node.dart';
import 'io.dart';
import 'logger.dart';
import 'logger/terse.dart';
import 'logger/deprecation_handling.dart';
import 'syntax.dart';
import 'utils.dart';
import 'visitor/async_evaluate.dart';
Expand All @@ -37,9 +38,14 @@ Future<CompileResult> compileAsync(String path,
bool quietDeps = false,
bool verbose = false,
bool sourceMap = false,
bool charset = true}) async {
TerseLogger? terseLogger;
if (!verbose) logger = terseLogger = TerseLogger(logger ?? Logger.stderr());
bool charset = true,
Set<Deprecation> fatalDeprecations = const {},
Set<Deprecation> futureDeprecations = const {}}) async {
DeprecationHandlingLogger deprecationLogger = logger =
DeprecationHandlingLogger(logger ?? Logger.stderr(),
fatalDeprecations: fatalDeprecations,
futureDeprecations: futureDeprecations,
limitRepetition: !verbose);

// If the syntax is different than the importer would default to, we have to
// parse the file manually and we can't store it in the cache.
Expand Down Expand Up @@ -71,7 +77,7 @@ Future<CompileResult> compileAsync(String path,
sourceMap,
charset);

terseLogger?.summarize(node: nodeImporter != null);
deprecationLogger.summarize(node: nodeImporter != null);
return result;
}

Expand All @@ -96,9 +102,14 @@ Future<CompileResult> compileStringAsync(String source,
bool quietDeps = false,
bool verbose = false,
bool sourceMap = false,
bool charset = true}) async {
TerseLogger? terseLogger;
if (!verbose) logger = terseLogger = TerseLogger(logger ?? Logger.stderr());
bool charset = true,
Set<Deprecation> fatalDeprecations = const {},
Set<Deprecation> futureDeprecations = const {}}) async {
DeprecationHandlingLogger deprecationLogger = logger =
DeprecationHandlingLogger(logger ?? Logger.stderr(),
fatalDeprecations: fatalDeprecations,
futureDeprecations: futureDeprecations,
limitRepetition: !verbose);

var stylesheet =
Stylesheet.parse(source, syntax ?? Syntax.scss, url: url, logger: logger);
Expand All @@ -118,7 +129,7 @@ Future<CompileResult> compileStringAsync(String source,
sourceMap,
charset);

terseLogger?.summarize(node: nodeImporter != null);
deprecationLogger.summarize(node: nodeImporter != null);
return result;
}

Expand Down
5 changes: 3 additions & 2 deletions lib/src/async_import_cache.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:path/path.dart' as p;
import 'package:tuple/tuple.dart';

import 'ast/sass.dart';
import 'deprecation.dart';
import 'importer.dart';
import 'importer/utils.dart';
import 'io.dart';
Expand Down Expand Up @@ -154,10 +155,10 @@ class AsyncImportCache {
? inImportRule(() => importer.canonicalize(url))
: importer.canonicalize(url));
if (result?.scheme == '') {
_logger.warn("""
_logger.warnForDeprecation(Deprecation.relativeCanonical, """
Importer $importer canonicalized $url to $result.
Relative canonical URLs are deprecated and will eventually be disallowed.
""", deprecation: true);
""");
}
return result;
}
Expand Down
31 changes: 21 additions & 10 deletions lib/src/compile.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// DO NOT EDIT. This file was generated from async_compile.dart.
// See tool/grind/synchronize.dart for details.
//
// Checksum: f8b5bf7eafbe3523ca4df1a6832e131c5c03986b
// Checksum: 6841a94168f5bcda5e026da4b1a8dbb5c0e1ff18
//
// ignore_for_file: unused_import

Expand All @@ -19,11 +19,12 @@ import 'ast/sass.dart';
import 'import_cache.dart';
import 'callable.dart';
import 'compile_result.dart';
import 'deprecation.dart';
import 'importer.dart';
import 'importer/legacy_node.dart';
import 'io.dart';
import 'logger.dart';
import 'logger/terse.dart';
import 'logger/deprecation_handling.dart';
import 'syntax.dart';
import 'utils.dart';
import 'visitor/evaluate.dart';
Expand All @@ -46,9 +47,14 @@ CompileResult compile(String path,
bool quietDeps = false,
bool verbose = false,
bool sourceMap = false,
bool charset = true}) {
TerseLogger? terseLogger;
if (!verbose) logger = terseLogger = TerseLogger(logger ?? Logger.stderr());
bool charset = true,
Set<Deprecation> fatalDeprecations = const {},
Set<Deprecation> futureDeprecations = const {}}) {
DeprecationHandlingLogger deprecationLogger = logger =
DeprecationHandlingLogger(logger ?? Logger.stderr(),
fatalDeprecations: fatalDeprecations,
futureDeprecations: futureDeprecations,
limitRepetition: !verbose);

// If the syntax is different than the importer would default to, we have to
// parse the file manually and we can't store it in the cache.
Expand Down Expand Up @@ -80,7 +86,7 @@ CompileResult compile(String path,
sourceMap,
charset);

terseLogger?.summarize(node: nodeImporter != null);
deprecationLogger.summarize(node: nodeImporter != null);
return result;
}

Expand All @@ -105,9 +111,14 @@ CompileResult compileString(String source,
bool quietDeps = false,
bool verbose = false,
bool sourceMap = false,
bool charset = true}) {
TerseLogger? terseLogger;
if (!verbose) logger = terseLogger = TerseLogger(logger ?? Logger.stderr());
bool charset = true,
Set<Deprecation> fatalDeprecations = const {},
Set<Deprecation> futureDeprecations = const {}}) {
DeprecationHandlingLogger deprecationLogger = logger =
DeprecationHandlingLogger(logger ?? Logger.stderr(),
fatalDeprecations: fatalDeprecations,
futureDeprecations: futureDeprecations,
limitRepetition: !verbose);

var stylesheet =
Stylesheet.parse(source, syntax ?? Syntax.scss, url: url, logger: logger);
Expand All @@ -127,7 +138,7 @@ CompileResult compileString(String source,
sourceMap,
charset);

terseLogger?.summarize(node: nodeImporter != null);
deprecationLogger.summarize(node: nodeImporter != null);
return result;
}

Expand Down