Backport deprecation API to legacy JS API #2293
Conversation
jathak
force-pushed
the
deprecation-backport
branch
from
617de35 to
dd8e82c
Compare
bin/sass.dart
Outdated
| logger: options.logger, | ||
| silenceDeprecations: options.silenceDeprecations, | ||
| fatalDeprecations: options.fatalDeprecations, | ||
| futureDeprecations: options.futureDeprecations, |
lib/src/deprecation.dart
Outdated
| @@ -6,6 +6,7 @@ import 'package:cli_pkg/js.dart'; | |||
| import 'package:collection/collection.dart'; | |||
| import 'package:pub_semver/pub_semver.dart'; | |||
|
|
|||
| import 'logger.dart'; | |||
lib/src/async_import_cache.dart
Outdated
| Logger? logger, | ||
| Set<Deprecation>? fatalDeprecations, | ||
| Set<Deprecation>? futureDeprecations, | ||
| Set<Deprecation>? silenceDeprecations}) |
There was a problem hiding this comment.
I don't really like this change. It's not actually providing any better guarantees—the caller still has to remember to explicitly handle the deprecation options for a new ImportCache—and it's reducing the separation of concerns between the way a Logger is configured and the way an ImportCache is configured. I'd rather just wrap the logger explicitly in the places it needs to be wrapped.
There was a problem hiding this comment.
Moved the wrapping for import cache into a static method so that it's now always wrapped explicitly, but we don't have to repeat the boilerplate (including the comment about not limiting repetition) in each place we wrap a logger this way.
There was a problem hiding this comment.
Should these constructor parameters be removed, then?
There was a problem hiding this comment.
Oh, whoops. I think I accidentally removed them from ImportCache instead of AsyncImportCache and then that got undone by synchronizing.
| this.limitRepetition = true}); | ||
|
|
||
| /// Warns if any of the deprecations options are incompatible or unnecessary. | ||
| void validate() { |
There was a problem hiding this comment.
Why split this out into a separate method?
There was a problem hiding this comment.
We only want to emit warnings for invalid/unnecessary deprecation flags once and we're now wrapping loggers more than once in many cases, so the warning part is separated out from the constructor and only called on the main wrapped logger.
See sass/sass#3907.
See sass/sass-spec#2006.
This also fixes a bug where the deprecation flags would not always be respected when using embedded Sass, as
ImportCaches could be created with an unwrapped logger, meaning any parse-time deprecation warnings (e.g.@import) couldn't be controlled.ImportCachenow wraps the logger it gets in aDeprecationProcessingLoggerto fix this.[skip sass-embedded]