Skip to content
This repository was archived by the owner on Jan 20, 2018. It is now read-only.

some fixes for strong mode. #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

some fixes for strong mode. #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f15a32
Merge pull request #1 from dart-lang/master
dam0vm3nt Aug 7, 2016
29a4fbd
strong mode fixes
dam0vm3nt Aug 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/build/html_import_annotation_recorder.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
library web_components.build.html_import_recorder_inliner;

import 'package:analyzer/src/generated/element.dart';
import 'package:analyzer/src/dart/constant/value.dart';
import 'package:initialize/transformer.dart';
import 'package:path/path.dart' as path;
import '../src/normalize_path.dart';
Expand Down Expand Up @@ -61,9 +62,9 @@ class HtmlImportAnnotationRecorder implements InitializerPlugin {

var originalImportPath;
if (annotationElement.element is PropertyAccessorElement) {
originalImportPath = resolver
originalImportPath = (resolver
.evaluateConstant(element.library, annotation.name)
.value
.value as DartObjectImpl)
.fields['filePath']
.toStringValue();
} else {
Expand Down
2 changes: 1 addition & 1 deletion lib/build/import_inliner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ void _moveHeadToWrapper(Document doc, Element wrapper) {
var foundImport = false;
for (var node in doc.head.nodes.toList(growable: false)) {
if (node is! Element) continue;
var tag = node.localName;
var tag = (node as Element).localName;
var type = node.attributes['type'];
var rel = node.attributes['rel'];
if (tag == 'link' && rel == 'import') foundImport = true;
Expand Down
6 changes: 3 additions & 3 deletions lib/build/script_compactor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ class ScriptCompactor {

Future<Asset> run() {
var crawler = new ImportCrawler(transform, primaryInput, logger);
return crawler.crawlImports().then((imports) {
return crawler.crawlImports().then((imports) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

This async/await stuff shouldn't be necessary afaik, if there is a strong mode error that is probably a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

analyzer wants a (...)- > Asset and instead it gets a (...) -> Future<Asset>. They shoud change Future.then signature to return a dynamic ? Should I send an issue to sdk or to analyzer to ignore it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like this is an issue with Future flattening? cc @jmesserly

Copy link
Contributor

@jmesserly jmesserly Aug 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

it's not a flattening bug, it is an inference issue dart-lang/sdk#25944, which I have a CL out, waiting for one more LGTM.

async is a good workaround, another good one is to pass the type arg explicitly like .then/*<Future<Asset>>*/(...)

Future extractScripts(id) =>
_extractInlineScripts(id, imports[id].document);

return Future.forEach(imports.keys, extractScripts).then((_) {
return await Future.forEach(imports.keys, extractScripts).then((_) async {
if (mainScript == null) {
logger.error(
exactlyOneScriptPerEntryPoint.create({'url': primaryInput.path}));
Expand All @@ -96,7 +96,7 @@ class ScriptCompactor {
assert(primaryDocument != null);

// Create the new bootstrap file and return its AssetId.
return _buildBootstrapFile(mainScript, importScripts);
return await _buildBootstrapFile(mainScript, importScripts);
});
});
}
Expand Down
4 changes: 2 additions & 2 deletions lib/build/web_components.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Asset generateWebComponentsBootstrap(Resolver resolver, Transform transform,
dom.Document document, AssetId scriptId, AssetId newScriptId,
{List<InitializerPlugin> extraPlugins: const []}) {
var htmlImportRecorder = new HtmlImportAnnotationRecorder();
var plugins = [htmlImportRecorder]..addAll(extraPlugins);
var plugins = [htmlImportRecorder as InitializerPlugin]..addAll(extraPlugins);
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary afaik, try specifying <InitializerPlugin>[htmlImportRecorder] instead? I think the actual issue here was it was creating a List<HtmlImportAnnotationRecorder> and that was failing when you tried to add the List<InitializerPlugin> to it.

cc @munificent @vsmenon , not sure if this thing comes up often for strong mode or its more of an outlier?

Copy link

Choose a reason for hiding this comment

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

Yeah, I think <InitializerPlugin>[htmlImportRecorder] is the cleanest solution here. If you don't put in the explicit type, strong mode infers it (for list and map literals) from the contents. In this case, it appears that is too strong - you want to add different plugins after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k worked. Actually I thought to have tried it before but probably I made some typos.


// Bootstrap the application using the `initialize` package and our
// plugins.
Expand All @@ -39,7 +39,7 @@ Asset generateWebComponentsBootstrap(Resolver resolver, Transform transform,
document.head.querySelector('script[type="application/dart"]');
for (var importPath in htmlImportRecorder.importPaths) {
var import = new dom.Element.tag('link')
..attributes = {'rel': 'import', 'href': importPath,};
..attributes = (new Map<dynamic,String>()..addAll({'rel': 'import', 'href': importPath,}));
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary either, and it should be a Map<String, String> afaik (see here). That type should be inferred though anyways, unless importPath isn't known to be a String or something?

Copy link

Choose a reason for hiding this comment

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

What was the error in the original code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[error] Type check failed: {'rel' : 'import', 'href' : importPath} (Map<String, String>) is not of type LinkedHashMap<dynamic, String> (package:web_components/build/web_components.dart, line 42, col 22)

Copy link
Contributor Author

@dam0vm3nt dam0vm3nt Aug 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

If I put an <dynamic,String> before the map literals I get this :

[error] Type check failed: <dynamic, String> {'rel' : 'import', 'href' : importPath} (Map<dynamic, String>) is not of type LinkedHashMap<dynamic, String> (package:web_components/build/web_components.dart, line 42, col 22)

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a bug as well, according to the api docs attributes should be a Map<String, String>

document.head.insertBefore(import, dartScript);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/src/mirror_initializer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class _State {
final Set<Document> seen = new Set();

/// Scripts that have been discovered, in tree order.
final LinkedHashMap<String, _ScriptInfo> scripts = {};
final LinkedHashMap<String, _ScriptInfo> scripts = new LinkedHashMap<String,_ScriptInfo>();
Copy link
Contributor

Choose a reason for hiding this comment

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

lets just change this to final scripts = <String, _ScriptInfo>{};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I get then:

[error] Type check failed: <String, _ScriptInfo> {} (Map<String, _ScriptInfo>) is not of type LinkedHashMap<String, _ScriptInfo> (package:web_components/src/mirror_initializer.dart, line 71, col 54)

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you remove the type on the left hand side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I missed that. Now it's ok.

}

/// Holds information about a Dart script tag.
Expand Down