-
Notifications
You must be signed in to change notification settings - Fork 350
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
Complete implementation the deprecations API #2207
Conversation
…tion handling logger
CHANGELOG.md
Outdated
* The deprecations API is now available in the JS API! The `compile` methods | ||
support the same `fatalDeprecations` and `futureDeprecations` options that | ||
were already available in the Dart API and the CLI, as well as a new | ||
`silenceDeprecations` option that allows you to silence deprecation warnings | ||
of a given type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fair bet that most JS API users don't know or follow the Dart API, so it's probably better to frame this as adding three new features to the JS API rather than bringing two from the Dart API and adding one new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
CHANGELOG.md
Outdated
|
||
### Command-Line Interface | ||
|
||
* Add a new `--silence-deprecation` flag to match the new JS API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, although to a lesser extent, the CLI users are a somewhat different set than the JS API users. Better to keep the CLI descriptions as self-contained as possible—it's less net effort for us to duplicate some prose than for a bunch of readers to read extra paragraphs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
CHANGELOG.md
Outdated
all deprecations, but will be used once some deprecations become obsolete in | ||
Dart Sass 2.0.0. | ||
|
||
* Fix a bug where `compileStringToResultAsync` ignored `fatalDeprecations` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd mark this as **Potentially breaking bug fix:**
on the off chance that someone was doing it without realizing that it wasn't failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/src/executable/options.dart
Outdated
for (var deprecation in Deprecation.values) | ||
if (deprecation.deprecatedIn != null && | ||
deprecation.description != null) | ||
deprecation.id: deprecation.description!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use if ... case
to avoid this nullability assertion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
} | ||
for (var deprecation in silenceDeprecations) { | ||
if (deprecation == Deprecation.userAuthored) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could make this block a switch statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Trace? trace, | ||
bool deprecation = false, | ||
Deprecation? deprecationType}) { | ||
if (deprecation && deprecationType != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A benefit of the old pattern of having a separate warnForDeprecation()
method is that it's clear from the static types that every deprecation warning has to have a deprecationType
attached to it. Unless there's a technical reason to fold that into warn()
, I think I'd prefer to keep it the old way. (If they do need to be merged, we should have error handling for what happens if deprecation
and deprecationType
disagree.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that there are other loggers that need to receive the deprecation type, using the same name for their method as the extension method meant that those other loggers could receive future deprecation warnings that hadn't been explicitly passed through by DeprecationProcessingLogger
, so I wanted to separate those names.
I've now updated the method that LoggerWithDeprecationType
s implement to be internalWarn
, which omits the boolean parameter and only has the Deprecation?
one. In Dart Sass 2.0.0, we can eliminate internalWarn
and LoggerWithDeprecationType
by just changing Logger.warn
's signature.
@@ -16,12 +16,15 @@ const _maxRepetitions = 5; | |||
|
|||
/// A logger that wraps an inner logger to have special handling for | |||
/// deprecation warnings. | |||
final class DeprecationHandlingLogger implements Logger { | |||
final class DeprecationHandlingLogger implements DeprecationLogger { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distinction between DeprecationLogger
and DeprecationHandlingLogger
is now pretty confusing, since neither the names nor the documentations clearly say what makes them different from one another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed this to DeprecationProcessingLogger
and fleshed out the documentation to make it clearer that this one processes deprecation warnings (sometimes ignoring them or treating them as errors) based on the various flags.
The other one is now LoggerWithDeprecationType
, which is clunky, but we can eliminate it in Dart Sass 2.0.0, so I'm not too worried about it.
test/embedded/utils.dart
Outdated
Iterable<String>? functions, | ||
Iterable<String>? fatalDeprecations, | ||
Iterable<String>? futureDeprecations, | ||
Iterable<String>? silenceDeprecations, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove trailing comma for consistency with other arglists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
test/embedded/utils.dart
Outdated
if (fatalDeprecations != null) { | ||
request.fatalDeprecation.addAll(fatalDeprecations); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider:
if (fatalDeprecations != null) { | |
request.fatalDeprecation.addAll(fatalDeprecations); | |
} | |
fatalDeprecations.andThen(request.fatalDeprecation.addAll); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/src/js/deprecations.dart
Outdated
}); | ||
|
||
jsClass.defineStaticMethod( | ||
'parse', (String version) => Version.parse(version)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a discrepancy with the spec here: the JS version API we've specified only supports <number>.<number>.<number>
versions, but Version.parse()
supports the full semver range, including build identifiers and prerelease versions. To match the spec, this should probably throw an error if anything beyond numbers are included.
lib/src/executable/options.dart
Outdated
for (var deprecation in Deprecation.values) | ||
if (deprecation | ||
case Deprecation( | ||
deprecatedIn: Version(), | ||
obsoleteIn: null, | ||
:var description? | ||
)) | ||
deprecation.id: description, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between this and --fatal-deprecation
, nearly half the length of the --help
output is taken up by deprecation descriptions. WDYT about just linking to the Sass site's CLI docs instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that makes more sense. Is there a short version of https://sass-lang.com/documentation/cli/dart-sass so we can link to it with #fatal-deprecation
, #silence-deprecation
and #future-deprecation
?
This implements the full deprecations API for the JS API and the embedded compiler, and also adds
silenceDeprecations
/--silence-deprecation
to the Dart API and CLI to complete those implementations.See sass/sass#3826
See sass/sass-spec#1967
[skip sass-embedded]