Strip bindings before calling Uri.parse

pkg/polymer/lib/src/build/import_inliner.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /// Transfomer that inlines polymer-element definitions from html imports.
 library polymer.src.build.import_inliner;
 
 import 'dart:async';
 import 'dart:convert';
 
@@ skipping 250 matching lines @@
 ///
 ///     myPkgName|lib/foo/bar.html -> myPkgName.foo.bar_html
 ///     myPkgName|web/foo/bar.html -> myPkgName.web.foo.bar_html
 ///
 /// This should roughly match the recommended library name conventions.
 String _libraryNameFor(AssetId id, int suffix) {
   var name = '${path.withoutExtension(id.path)}_'
       '${path.extension(id.path).substring(1)}';
   if (name.startsWith('lib/')) name = name.substring(4);
   name = name.split('/').map((part) {
-    part = part.replaceAll(INVALID_LIB_CHARS_REGEX, '_');
-    if (part.startsWith(NUM_REGEX)) part = '_${part}';
+    part = part.replaceAll(_INVALID_LIB_CHARS_REGEX, '_');
+    if (part.startsWith(_NUM_REGEX)) part = '_${part}';
     return part;
   }).join(".");
   return '${id.package}.${name}_$suffix';
 }
 
 /// Parse [code] and determine whether it has a library directive.
 bool _hasLibraryDirective(String code) =>
     parseDirectives(code, suppressErrors: true)
         .directives.any((d) => d is LibraryDirective);
 
@@ skipping 53 matching lines @@
     return changed;
   }
 
   visitElement(Element node) {
     // TODO(jakemac): Support custom elements that extend html elements which
     // have url-like attributes. This probably means keeping a list of which
     // html elements support each url-like attribute.
     if (!isCustomTagName(node.localName)) {
       node.attributes.forEach((name, value) {
         if (_urlAttributes.contains(name)) {
-          if (!name.startsWith('_') && value.contains(_BINDINGS)) {
+          if (!name.startsWith('_') && value.contains(_BINDING_REGEX)) {
             transform.logger.warning(
                 'When using bindings with the "$name" attribute you may '
                 'experience errors in certain browsers. Please use the '
                 '"_$name" attribute instead. For more information, see '
                 'http://goo.gl/5av8cU', span: node.sourceSpan, asset: sourceId);
-          } else if (name.startsWith('_') && !value.contains(_BINDINGS)) {
+          } else if (name.startsWith('_') && !value.contains(_BINDING_REGEX)) {
             transform.logger.warning(
                 'The "$name" attribute is only supported when using bindings. '
                 'Please change to the "${name.substring(1)}" attribute.',
                 span: node.sourceSpan, asset: sourceId);
           }
-          if (value != '' && !value.trim().startsWith(_BINDINGS)) {
+          if (value != '' && !value.trim().startsWith(_BINDING_REGEX)) {
             node.attributes[name] = _newUrl(value, node.sourceSpan);
             changed = changed || value != node.attributes[name];
           }
         }
       });
     }
     if (node.localName == 'style') {
       node.text = visitCss(node.text);
       changed = true;
     } else if (node.localName == 'script' &&
         node.attributes['type'] == TYPE_DART &&
         !node.attributes.containsKey('src')) {
       // TODO(jmesserly): we might need to visit JS too to handle ES Harmony
       // modules.
       node.text = visitInlineDart(node.text);
       changed = true;
     }
     return super.visitElement(node);
   }
 
   static final _URL = new RegExp(r'url\(([^)]*)\)', multiLine: true);
   static final _QUOTE = new RegExp('["\']', multiLine: true);
-  static final _BINDINGS = new RegExp(r'({{)|(\[\[)');
 
   /// Visit the CSS text and replace any relative URLs so we can inline it.
   // Ported from:
   // https://github.com/Polymer/vulcanize/blob/c14f63696797cda18dc3d372b78aa3378acc691f/lib/vulcan.js#L149
   // TODO(jmesserly): use csslib here instead? Parsing with RegEx is sadness.
   // Maybe it's reliable enough for finding URLs in CSS? I'm not sure.
   String visitCss(String cssText) {
     var url = spanUrlFor(sourceId, transform);
     var src = new SourceFile(cssText, url: url);
     return cssText.replaceAllMapped(_URL, (match) {
@@ skipping 36 matching lines @@
     }
 
     if (!output.hasEdits) return code;
 
     // TODO(sigmund): emit source maps when barback supports it (see
     // dartbug.com/12340)
     return (output.commit()..build(file.url.toString())).text;
   }
 
   String _newUrl(String href, SourceSpan span) {
-    // Uri.parse blows up on invalid characters (like {{). Encoding the uri
-    // allows it to be parsed, which does the correct thing in the general case.
-    // This uri not used to build the new uri, so it never needs to be decoded.
-    var uri = Uri.parse(Uri.encodeFull(href));
+    // We only want to parse the part of the href leading up to the first
+    // binding, anything after that is not informative.
+    var hrefToParse;
+    var firstBinding = href.indexOf(_BINDING_REGEX);
+    if (firstBinding == -1) {
+      hrefToParse = href;
+    } else if (firstBinding == 0) {
+      return href;
+    } else {
+      hrefToParse = '${href.substring(0, firstBinding)}$_BINDING_PLACEHOLDER';
+    }
+
+    var uri = Uri.parse(hrefToParse);
     if (uri.isAbsolute) return href;
     if (!uri.scheme.isEmpty) return href;
     if (!uri.host.isEmpty) return href;
     if (uri.path.isEmpty) return href;  // Implies standalone ? or # in URI.
     if (path.isAbsolute(href)) return href;
 
-    var id = uriToAssetId(sourceId, href, transform.logger, span);
+    var id = uriToAssetId(sourceId, hrefToParse, transform.logger, span);
     if (id == null) return href;
     var primaryId = transform.primaryInput.id;
 
-    if (id.path.startsWith('lib/')) {
-      return '${topLevelPath}packages/${id.package}/${id.path.substring(4)}';
+    // Build the new path, placing back any suffixes that we stripped earlier.
+    var prefix = (firstBinding < 1) ? id.path

[Siggi Cherem (dart-lang), 2014/08/04 19:39:38]

nit: since we return automatically with == 0 upfront, I am debating whether it
reads slightly better to use `== -1` here (as normally it means there were no
matches).

[jakemac, 2014/08/04 19:55:01]

Done, I think it is a bit better

+        : id.path.substring(0, id.path.length - _BINDING_PLACEHOLDER.length);
+    var suffix = (firstBinding < 1) ? '' : href.substring(firstBinding);
+    var newPath = '$prefix$suffix';
+
+    if (newPath.startsWith('lib/')) {
+      return '${topLevelPath}packages/${id.package}/${newPath.substring(4)}';
     }
 
-    if (id.path.startsWith('asset/')) {
-      return '${topLevelPath}assets/${id.package}/${id.path.substring(6)}';
+    if (newPath.startsWith('asset/')) {
+      return '${topLevelPath}assets/${id.package}/${newPath.substring(6)}';
     }
 
     if (primaryId.package != id.package) {
       // Techincally we shouldn't get there
       transform.logger.error("don't know how to include $id from $primaryId",
           span: span);
       return href;
     }
 
     var builder = path.url;
-    return builder.relative(builder.join('/', id.path),
+    return builder.relative(builder.join('/', newPath),
         from: builder.join('/', builder.dirname(primaryId.path)));
   }
 }
 
 /// HTML attributes that expect a URL value.
 /// <http://dev.w3.org/html5/spec/section-index.html#attributes-1>
 ///
 /// Every one of these attributes is a URL in every context where it is used in
 /// the DOM. The comments show every DOM element where an attribute can be used.
 ///
@@ skipping 10 matching lines @@
   'poster', '_poster',          // in video
   'src', '_src',                // in audio, embed, iframe, img, input, script,
                                 //    source, track,video
 ];
 
 /// When inlining <link rel="stylesheet"> tags copy over all attributes to the
 /// style tag except these ones.
 const IGNORED_LINKED_STYLE_ATTRS =
     const ['charset', 'href', 'href-lang', 'rel', 'rev'];
 
-/// Global RegExp objects for validating generated library names.
-final INVALID_LIB_CHARS_REGEX = new RegExp('[^a-z0-9_]');
-final NUM_REGEX = new RegExp('[0-9]');
+/// Global RegExp objects.
+final _INVALID_LIB_CHARS_REGEX = new RegExp('[^a-z0-9_]');
+final _NUM_REGEX = new RegExp('[0-9]');
+final _BINDING_REGEX = new RegExp(r'(({{.*}})|(\[\[.*\]\]))');
+
+/// Placeholder for bindings in urls for [Uri.parse].
+final _BINDING_PLACEHOLDER = '_';

[Siggi Cherem (dart-lang), 2014/08/04 19:39:38]

nit: maybe rename or rephrase the comment a big ("placeholder or the first
binding.. " ?)

alternatively, since we use this only in_newUrl, we could move the definition up
to the method and make it a const local variable instead, e.g.:

   const placeholder = '_';

[jakemac, 2014/08/04 19:55:01]

Done, went with the const local

 
 _getSpan(SourceFile file, AstNode node) => file.span(node.offset, node.end);