-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Ambient Directives ♡ Dart Transformers #5129
Conversation
I think we should change the format by which ambient directives are declared. My suggestions:
I'll make comments in the PR for where I think these changes could be made. |
@@ -18,6 +18,7 @@ TransformerOptions parseBarbackSettings(BarbackSettings settings) { | |||
config, REFLECT_PROPERTIES_AS_ATTRIBUTES_OLD, | |||
defaultValue: false); | |||
} | |||
var ambientDirectives = config[AMBIENT_DIRECTIVES]; |
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.
Use (and please rename) _readFileList
to smooth over differences between single values & List
s
b89edce
to
a3545b5
Compare
@@ -99,6 +103,49 @@ class _CompileDataCreator { | |||
return new CompileDataResults._(ngMeta, compileData); | |||
} | |||
|
|||
Future<List<CompileDirectiveMetadata>> _readAmbientDirectives() async { | |||
if (ambientDirectives == null) return []; |
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: return const []
Do you have an objection to point 1 above? If so let's discuss, otherwise PTAL. |
a3545b5
to
55c7df2
Compare
Changed the format to be "package:/library.dart#CORE_DIRECTIVES". |
@@ -39,6 +40,10 @@ class TransformerOptions { | |||
/// as attributes on DOM elements, which may aid in application debugging. | |||
final bool reflectPropertiesAsAttributes; | |||
|
|||
/// A set of directives that will be automatically passed-in to the template compiler | |||
/// Format of an item in the list: angular2/lib/src/core/directives.dart:CORE_DIRECTIVES |
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.
Update comment
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.
fixed
55c7df2
to
4cae870
Compare
return newMetadata.flatten(token); | ||
|
||
} else { | ||
logger.warning('Could not resolve ${token} in ${uri}'); |
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.
Add a little more detail here:
logger.warning('Could not resolve ambient directive ${token} in ${uri}',
asset: metaAssetId);
4cae870
to
731e1ea
Compare
731e1ea
to
153114e
Compare
Merging PR #5129 on behalf of @vsavkin to branch presubmit-vsavkin-pr-5129. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.