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

jathak · 2022-10-28T20:01:16Z

Partial fix for #633 and #1363.

This adds a new Deprecation enum to track the different deprecations that are ongoing or upcoming and allow users to select certain deprecations to treat as fatal (or future deprecations to begin warning for now).

This adds support to the CLI and the Dart API. It does not add support for the JS API yet, but I'd like to land this first so the underlying logic isn't blocked on the language change process.

This adds a new `Deprecation` class that specifies an ID for each deprecated feature along with what Dart Sass version deprecated it. The compile functions allow you to pass a set of `fatalDeprecations` that will cause an error instead of a warning. You can also pass a set of `futureDeprecations`, which let you opt-in to deprecations (like `@import`) early.

nex3

Can you also file issues to track adding this to the JS API and documenting it on the Sass website?

CHANGELOG.md

nex3 · 2023-03-04T01:58:54Z

CHANGELOG.md

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


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

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)

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?

nex3 · 2023-03-04T02:01:29Z

bin/sass.dart

+        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)));


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?

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?

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

lib/sass.dart

lib/src/deprecation.dart

nex3 · 2023-03-04T02:11:34Z

lib/src/executable/options.dart

+      ..addMultiOption('fatal-deprecation',
+          help: 'Deprecations to treat as errors. You may also pass a Sass\n'
+              'version to include any behavior deprecated in or before it.\n'
+              'See https://sass-lang.com/documentation/breaking-changes for \n'


We should update this documentation so that each breaking change includes an example of how to make it fatal as well

Working on a sass-site PR now

sass/sass-site#702

lib/src/executable/options.dart

lib/src/logger.dart

lib/src/logger/deprecation_handling.dart

lib/src/deprecation.dart

nex3 · 2023-03-06T21:22:00Z

lib/src/deprecation.dart

+  /// For deprecations that have existed in all versions of Dart Sass, this
+  /// should be 0.0.0. For deprecations that are not yet active, this should be
+  /// null.
+  final String? deprecatedIn;


Oh, I see... real enums strike again. I think the cleanest option given that might be to convert this to a normal class with static final members instead of a literal enum. That's more work to declare, but nicer on the consuming end, which I think it a worthy trade-off.

nex3 · 2023-03-06T21:24:13Z

CHANGELOG.md

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


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?

nex3 · 2023-03-06T21:25:11Z

bin/sass.dart

+        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)));


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

lib/src/logger/deprecation_handling.dart

nex3 · 2023-03-06T21:27:06Z

lib/src/deprecation.dart

+  /// If this is true, `deprecatedIn` should be null, since we do not yet know
+  /// what version of Dart Sass this deprecation will be live in.


If you make this a real class, you can also verify this in a constructor assert().

Accomplished this in the enum form by having separate constructors for regular and future deprecations.

jathak added 9 commits October 28, 2022 12:17

Add future deprecation for @import

8f3dae1

Add flags

323aa1c

Merge colorUnits and randomWithUnits

e3f1e9e

Update changelogs and pubspecs

96df02f

Merge remote-tracking branch 'origin/main' into deprecations

d1f28f5

Merge remote-tracking branch 'origin/main' into deprecations

7e23ec8

Add tests

6b3e6f0

Merge remote-tracking branch 'origin/main' into deprecations

d00498c

jathak marked this pull request as ready for review March 4, 2023 00:57

jathak requested a review from nex3 March 4, 2023 00:57

nex3 requested changes Mar 4, 2023

View reviewed changes

Code review

a4bc07d

jathak requested a review from nex3 March 6, 2023 18:51

Use isFuture instead of deprecatedIn == null

a3a8974

nex3 reviewed Mar 6, 2023

View reviewed changes

jathak added 3 commits March 6, 2023 14:21

More code review

1d38e63

Merge remote-tracking branch 'origin/main' into deprecations

b03ac1d

Split warnForDeprecation from warn

91c23dd

jathak requested a review from nex3 March 9, 2023 23:41

nex3 approved these changes Mar 9, 2023

View reviewed changes

Add missing word to error message

2b99fef

jathak merged commit 8f8138d into main Mar 10, 2023

jathak deleted the deprecations branch March 10, 2023 22:24

nex3 mentioned this pull request Mar 20, 2023

Add a --silence-deprecations flag #1918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jathak commented Oct 28, 2022 •

edited

nex3 left a comment

nex3 Mar 4, 2023

jathak Mar 6, 2023

nex3 Mar 6, 2023

jathak Mar 9, 2023

nex3 Mar 4, 2023

jathak Mar 6, 2023

nex3 Mar 6, 2023

jathak Mar 6, 2023

nex3 Mar 4, 2023

jathak Mar 6, 2023

jathak Mar 9, 2023

nex3 Mar 6, 2023

nex3 Mar 6, 2023

nex3 Mar 6, 2023

nex3 Mar 6, 2023

jathak Mar 6, 2023

		/// If this is true, `deprecatedIn` should be null, since we do not yet know
		/// what version of Dart Sass this deprecation will be live in.

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

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

Conversation

jathak commented Oct 28, 2022 • edited

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

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

jathak commented Oct 28, 2022 •

edited