Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22825012:
Introduce transformers for: (Closed)
|
Created:
7 years, 4 months ago by Siggi Cherem (dart-lang) Modified:
7 years, 3 months ago CC:
reviews_dartlang.org, web-ui-dev+reviews_dartlang.org Base URL:
git@github.com:dart-lang/web-ui.git@master Visibility:
Public. |
DescriptionIntroduce transformers for:
- inlined code extraction
- inlining html imports
- combining multiple script tags
See http://dartbug.com/12511
Patch Set 1 #Patch Set 2 : #
Total comments: 47
Messages
Total messages: 13 (0 generated)
John/Terry - here are a couple phases of our new deploy story. Terry, we should talk later this week to discuss about how we are going to build the css transformers. Bob, this is mostly polymer code, but you might be interested in looking at the way we use the Transformer API and the tests. https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/code... File test/transform/code_extractor_test.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/code... test/transform/code_extractor_test.dart:15: testPhases('no changes', [[new InlineCodeExtractor()]], { this is defined in 'common.dart', basically read it as: [[transformer]], input_files, output_files
I just skimmed it, but this looks really cool! There's definitely a few things we can do in barback to make this easier: 1. Make the primary input's asset synchronously available. 2. Get rid of isPrimary and just roll it into transform(). That way, you don't have to output the input file unmodified when you don't care about it. 3. Have some default behavior for filtering based on file extension. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/code_extractor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:24: return getPrimaryContent(transform).then((content) { Add a TODO mentioning #12515 or #12516 here since barback should make this easier. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:42: count == 0 ? content : document.outerHtml)); Just indent +4 here or all the way to the "(". https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/common.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:70: } Pub serve has identical parsing code for this. We should probably move it into barback so you can just call it directly. File a bug and add a TODO? https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... File lib/src/transform/import_inliner.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... lib/src/transform/import_inliner.dart:49: * [element], unless they have already been [seen]. Elements are added in the "[element]" -> "[elements]".
https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/code_extractor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:10: import 'package:path/path.dart' as path; not sure if you want to sort these, noticed a few places they aren't. doesn't matter to me FWIW https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:35: var id = inputId.addExtension('.$count.dart'); do we need to worry about this name conflicting with an existing file? or does barback handle this? https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/common.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:28: var parser = new HtmlParser(contents, generateSpans: true, One recurring issue in DWC was the inability to supply the "transport level encoding". Ideally, in their "build file" or whatever the barback equivalent is, the user can configure that their server is using "utf-8" and therefore html5lib does not need to detect encoding. Is that possible? An alternative would be to make the "HtmlInputStream.defaultEncoding" a parameter of html5lib, so if no other encoding is found (BOM, meta charset) it will optimistically assume utf-8 instead of pessimistically assuming windows-1252. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:34: for (var e in parser.errors) { suggestion: filter out the following error: if (isHtmlImport && e.errorCode == 'expected-doctype-but-got-start-tag') continue; https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:42: if (url == null || url == '') return null; generally, it's nice if only one of these is allowed as the sentinel value https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:45: logger.error('absolute paths not allowed here: "$url"', span); what is "here" :) https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... File lib/src/transform/import_inliner.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... lib/src/transform/import_inliner.dart:15: // TODO(sigmund): what about other things in html-imports, like scripts? inline *all the things* \o/ https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... lib/src/transform/import_inliner.dart:41: document.body.nodes.length == 0 ? null : document.body.nodes[0]); TODO(jmesserly): add Node.firstChild to html5lib http://api.dartlang.org/docs/bleeding_edge/dart_html/Node.html#firstChild https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... lib/src/transform/import_inliner.dart:84: return _visitImports(document, id, transform, seen, elements) this might look better with => https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... File lib/src/transform/script_compactor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:47: logger.warning('unexpected script without a src url', tag.sourceSpan); Hmm, why the warning here? Are we assuming the script extractor was run first? If so, maybe it should call the other one to ensure that phase as a pre-req. Does barback have such a concept? worse case, you could mention the other tranformer in the warning message. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:52: // TODO(sigmund): should we detect/remove duplicates? i think so. don't want to generate erroneous code that imports the same library twice. however, we probably should run the main twice. Try running foo: <script src="foo.dart"></script> Try running foo again: <script src="foo.dart"></script> foo.dart: main() => print('FOO!'); https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:119: currentMirrorSystem().findLibrary(const Symbol('app_bootstrap')) seems unfortunate to need to use mirrors for this. couldn't we get the HTML's base URL? https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/code... File test/transform/code_extractor_test.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/code... test/transform/code_extractor_test.dart:15: testPhases('no changes', [[new InlineCodeExtractor()]], { On 2013/08/19 21:23:38, Siggi Cherem (dart-lang) wrote: > this is defined in 'common.dart', basically read it as: > > [[transformer]], input_files, output_files why not [[[[[transformer]]]]]? ;) https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/scri... File test/transform/script_compactor_test.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/scri... test/transform/script_compactor_test.dart:33: 'library app_bootstrap;\n\n' this would be a good use case for triple quote :) ... (either left justified, or .replaceAll('\n ', '\n') works for me)
https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/code... File test/transform/code_extractor_test.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/code... test/transform/code_extractor_test.dart:15: testPhases('no changes', [[new InlineCodeExtractor()]], { On 2013/08/19 22:57:26, John Messerly wrote: > On 2013/08/19 21:23:38, Siggi Cherem (dart-lang) wrote: > > this is defined in 'common.dart', basically read it as: > > > > [[transformer]], input_files, output_files > > why not [[[[[transformer]]]]]? ;) In case you're actually curious... Barback organizes transformers in a series of phases. Each transform can only see the outputs of previous phases. Each phase can have multiple transformers which can be run in parallel. The outer list is the phases, the inner is the transformers in that phase. So [[transformer]] is one phase with one transformer, [[a], [b]] is two transformers in series, and [[a, b]] is two parallel transformers.
Saw each of the transforms but I'm assuming the order in which a transform is run hasn't been done yet (or no way to signal order in barback)? https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/code_extractor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:18: class InlineCodeExtractor extends Transformer { Does Barback has the ability to pass options for things like strict mode, etc. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:36: transform.addOutput(new Asset.fromString(id, textContent.value)); Can barback produce an output stream. Might want the ability to produce a file name for referring in other files or an optional filename filter to produce a file name of a particular extension https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... File lib/src/transform/script_compactor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:14: import 'code_extractor.dart'; // import just for documentation. If this import isn't used the editor will flag a warning.
> Saw each of the transforms but I'm assuming the order in which a transform is run hasn't been done yet (or no way to signal order in barback)? Transformers are ordered in phases. When barback requests the transformers for a given package, it is returned them ordered by phase. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/code_extractor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:18: class InlineCodeExtractor extends Transformer { On 2013/08/20 17:03:24, terry wrote: > Does Barback has the ability to pass options for things like strict mode, etc. No, it doesn't currently have a way to handle configuration. (Or another way of looking at it is it expects the transformers you give it to already be configured.) It's on the roadmap. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:36: transform.addOutput(new Asset.fromString(id, textContent.value)); On 2013/08/20 17:03:24, terry wrote: > Can barback produce an output stream. Yes, a barback Asset exposes a stream interface you can use to read its contents. > Might want the ability to produce a file > name for referring in other files or an optional filename filter to produce a > file name of a particular extension Assets also have file paths, even though they may not be physically on the file system.
https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/common.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:28: var parser = new HtmlParser(contents, generateSpans: true, it sounds like Bob is saying barback has no config options right now. I added a "defaultEncoding" to html5lib but in trying to write the doc comment, I couldn't really justify when to use it over "encoding". Here's my suggestion, let's just add "TODO(jmesserly): make HTTP encoding configurable" and for now assume the server is sending charset=utf-8. Based on this blog post from 2012: http://googleblog.blogspot.com/2012/02/unicode-over-60-percent-of-web.html, Unicode has already taken over, and utf-8 is the most popular unicode encoding, so I think it's a safe bet.
Thanks everyone. Now that polymer moved to the SVN repo, I moved the files of this CL there too. I created a new CL at: https://codereview.chromium.org/22935016/. To make it easier to review, I made the new CL with 2 patchets. PatchSet 1 matches what this CL had, PatchSet 2 addresses the comments below. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/code_extractor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:10: import 'package:path/path.dart' as path; On 2013/08/19 22:57:26, John Messerly wrote: > not sure if you want to sort these, noticed a few places they aren't. doesn't > matter to me FWIW Done. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:24: return getPrimaryContent(transform).then((content) { On 2013/08/19 22:28:59, Bob Nystrom wrote: > Add a TODO mentioning #12515 or #12516 here since barback should make this > easier. awesome. Done. moved the TODOs to the definition of these functions. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:35: var id = inputId.addExtension('.$count.dart'); On 2013/08/19 22:57:26, John Messerly wrote: > do we need to worry about this name conflicting with an existing file? or does > barback handle this? It's mainly on our hands to worry about finding a unique asset id in these cases. Barback helps in that collisions are not overriding files in the file system, but we have to be careful not choose something that already exists. This current scheme in this code assumes that users are not calling their files foo.html.0.dart. Bob - would it make sense to provide something extra in barback to guarantee uniqueness, for example a transformer or phase identifier that can be added to the asset id? https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:42: count == 0 ? content : document.outerHtml)); On 2013/08/19 22:28:59, Bob Nystrom wrote: > Just indent +4 here or all the way to the "(". Done. Thanks, vi-mode for Dart is not perfect yet =) https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/common.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:28: var parser = new HtmlParser(contents, generateSpans: true, On 2013/08/20 17:50:56, John Messerly wrote: > it sounds like Bob is saying barback has no config options right now. > > I added a "defaultEncoding" to html5lib but in trying to write the doc comment, > I couldn't really justify when to use it over "encoding". > > Here's my suggestion, let's just add "TODO(jmesserly): make HTTP encoding > configurable" and for now assume the server is sending charset=utf-8. Based on > this blog post from 2012: > http://googleblog.blogspot.com/2012/02/unicode-over-60-percent-of-web.html, > Unicode has already taken over, and utf-8 is the most popular unicode encoding, > so I think it's a safe bet. Done, added the utf8 argument as a default encoding. Is that what you meant? https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:34: for (var e in parser.errors) { On 2013/08/19 22:57:26, John Messerly wrote: > suggestion: filter out the following error: > > if (isHtmlImport && e.errorCode == 'expected-doctype-but-got-start-tag') > continue; > Done. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:42: if (url == null || url == '') return null; On 2013/08/19 22:57:26, John Messerly wrote: > generally, it's nice if only one of these is allowed as the sentinel value Not sure I follow - in this case this is more checking for invalid input. We could check for this on the call site, but I think resolve should be robust enough to handle this correctly. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:45: logger.error('absolute paths not allowed here: "$url"', span); On 2013/08/19 22:57:26, John Messerly wrote: > what is "here" :) "here" is now gone :) https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/common.dart:70: } On 2013/08/19 22:28:59, Bob Nystrom wrote: > Pub serve has identical parsing code for this. We should probably move it into > barback so you can just call it directly. File a bug and add a TODO? Done. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... File lib/src/transform/import_inliner.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... lib/src/transform/import_inliner.dart:15: // TODO(sigmund): what about other things in html-imports, like scripts? On 2013/08/19 22:57:26, John Messerly wrote: > inline *all the things* \o/ :-) filed a bug and updated the TODO with more details. We should probably update this and boot.js at the same time. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... lib/src/transform/import_inliner.dart:41: document.body.nodes.length == 0 ? null : document.body.nodes[0]); On 2013/08/19 22:57:26, John Messerly wrote: > TODO(jmesserly): add Node.firstChild to html5lib > http://api.dartlang.org/docs/bleeding_edge/dart_html/Node.html#firstChild Done. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... lib/src/transform/import_inliner.dart:49: * [element], unless they have already been [seen]. Elements are added in the On 2013/08/19 22:28:59, Bob Nystrom wrote: > "[element]" -> "[elements]". Done. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/i... lib/src/transform/import_inliner.dart:84: return _visitImports(document, id, transform, seen, elements) On 2013/08/19 22:57:26, John Messerly wrote: > this might look better with => unfortunatelly I need the 'document' argument both here and in the next call below. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... File lib/src/transform/script_compactor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:14: import 'code_extractor.dart'; // import just for documentation. On 2013/08/20 17:03:24, terry wrote: > If this import isn't used the editor will flag a warning. the good news is that they also recognize that the import is used for the dart doc comment, so they don't complain =), yay! (I just doubled checked) https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:47: logger.warning('unexpected script without a src url', tag.sourceSpan); On 2013/08/19 22:57:26, John Messerly wrote: > Hmm, why the warning here? Are we assuming the script extractor was run first? > If so, maybe it should call the other one to ensure that phase as a pre-req. > Does barback have such a concept? > > worse case, you could mention the other tranformer in the warning message. Good question. The transformers should always be run in order (later I'll set them up in phases) but I wanted to make each transformer is robust enough to catch if their preconditions are invalidated. I'm not sure what the right action should be in that case. Should we issue a warning, an error, or do something else? For now I kept the warning but I referred to the InlineCodeExtractor as you suggested. https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:52: // TODO(sigmund): should we detect/remove duplicates? On 2013/08/19 22:57:26, John Messerly wrote: > i think so. don't want to generate erroneous code that imports the same library > twice. however, we probably should run the main twice. > > Try running foo: > <script src="foo.dart"></script> > Try running foo again: > <script src="foo.dart"></script> > > foo.dart: > main() => print('FOO!'); But then, wouldn't it be ok to import it twice? (we anyways import them with a prefix) https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:119: currentMirrorSystem().findLibrary(const Symbol('app_bootstrap')) On 2013/08/19 22:57:26, John Messerly wrote: > seems unfortunate to need to use mirrors for this. couldn't we get the HTML's > base URL? good question. I opened a bug to investigate. https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/scri... File test/transform/script_compactor_test.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/test/transform/scri... test/transform/script_compactor_test.dart:33: 'library app_bootstrap;\n\n' On 2013/08/19 22:57:26, John Messerly wrote: > this would be a good use case for triple quote :) ... (either left justified, or > .replaceAll('\n ', '\n') works for me) Done.
https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... File lib/src/transform/code_extractor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... lib/src/transform/code_extractor.dart:35: var id = inputId.addExtension('.$count.dart'); On 2013/08/21 20:35:42, Siggi Cherem (dart-lang) wrote: > On 2013/08/19 22:57:26, John Messerly wrote: > > do we need to worry about this name conflicting with an existing file? or does > > barback handle this? > > It's mainly on our hands to worry about finding a unique asset id in these > cases. > > Barback helps in that collisions are not overriding files in the file system, > but we have to be careful not choose something that already exists. Similarly, you need to choose something that is not the output of a sibling transformer. You can collide with generated stuff too. > This current > scheme in this code assumes that users are not calling their files > foo.html.0.dart. > > Bob - would it make sense to provide something extra in barback to guarantee > uniqueness, for example a transformer or phase identifier that can be added to > the asset id? I don't think we want to add more state to AssetID, but it might be good to have an API that will generate a fresh unique-ish ID for you. I'm not sure exactly what that would look like, though. Want to just sit on this for now and see if it becomes a problem?
On 2013/08/21 21:18:21, Bob Nystrom wrote: > https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... > File lib/src/transform/code_extractor.dart (right): > > https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... > lib/src/transform/code_extractor.dart:35: var id = > inputId.addExtension('.$count.dart'); > On 2013/08/21 20:35:42, Siggi Cherem (dart-lang) wrote: > > On 2013/08/19 22:57:26, John Messerly wrote: > > > do we need to worry about this name conflicting with an existing file? or > does > > > barback handle this? > > > > It's mainly on our hands to worry about finding a unique asset id in these > > cases. > > > > Barback helps in that collisions are not overriding files in the file system, > > but we have to be careful not choose something that already exists. > > Similarly, you need to choose something that is not the output of a sibling > transformer. You can collide with generated stuff too. > > > This current > > scheme in this code assumes that users are not calling their files > > foo.html.0.dart. > > > > Bob - would it make sense to provide something extra in barback to guarantee > > uniqueness, for example a transformer or phase identifier that can be added to > > the asset id? > > I don't think we want to add more state to AssetID, but it might be good to have > an API that will generate a fresh unique-ish ID for you. I'm not sure exactly > what that would look like, though. Want to just sit on this for now and see if > it becomes a problem? I'm ok with that, should I file a low-pri bug on it to keep track of it?
On 2013/08/21 22:10:49, Siggi Cherem (dart-lang) wrote: > On 2013/08/21 21:18:21, Bob Nystrom wrote: > > > https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... > > File lib/src/transform/code_extractor.dart (right): > > > > > https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/c... > > lib/src/transform/code_extractor.dart:35: var id = > > inputId.addExtension('.$count.dart'); > > On 2013/08/21 20:35:42, Siggi Cherem (dart-lang) wrote: > > > On 2013/08/19 22:57:26, John Messerly wrote: > > > > do we need to worry about this name conflicting with an existing file? or > > does > > > > barback handle this? > > > > > > It's mainly on our hands to worry about finding a unique asset id in these > > > cases. > > > > > > Barback helps in that collisions are not overriding files in the file > system, > > > but we have to be careful not choose something that already exists. > > > > Similarly, you need to choose something that is not the output of a sibling > > transformer. You can collide with generated stuff too. > > > > > This current > > > scheme in this code assumes that users are not calling their files > > > foo.html.0.dart. > > > > > > Bob - would it make sense to provide something extra in barback to guarantee > > > uniqueness, for example a transformer or phase identifier that can be added > to > > > the asset id? > > > > I don't think we want to add more state to AssetID, but it might be good to > have > > an API that will generate a fresh unique-ish ID for you. I'm not sure exactly > > what that would look like, though. Want to just sit on this for now and see if > > it becomes a problem? > > I'm ok with that, should I file a low-pri bug on it to keep track of it? SGTM!
https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... File lib/src/transform/script_compactor.dart (right): https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:52: // TODO(sigmund): should we detect/remove duplicates? On 2013/08/21 20:35:42, Siggi Cherem (dart-lang) wrote: > On 2013/08/19 22:57:26, John Messerly wrote: > > i think so. don't want to generate erroneous code that imports the same > library > > twice. however, we probably should run the main twice. > > > > Try running foo: > > <script src="foo.dart"></script> > > Try running foo again: > > <script src="foo.dart"></script> > > > > foo.dart: > > main() => print('FOO!'); > > But then, wouldn't it be ok to import it twice? (we anyways import them with a > prefix) good point, importing twice would be fine assuming it doesn't trigger a Dart language level error. if they are imported with unique prefixes that should work great :) https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... lib/src/transform/script_compactor.dart:119: currentMirrorSystem().findLibrary(const Symbol('app_bootstrap')) On 2013/08/21 20:35:42, Siggi Cherem (dart-lang) wrote: > On 2013/08/19 22:57:26, John Messerly wrote: > > seems unfortunate to need to use mirrors for this. couldn't we get the HTML's > > base URL? > > good question. I opened a bug to investigate. btw, I discovered another pattern using mirrors that is slightly simpler (doesn't depend on findLibrary): currentMirrorSystem().isolate.rootLibrary.uri
On 2013/08/21 23:35:28, John Messerly wrote: > https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... > File lib/src/transform/script_compactor.dart (right): > > https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... > lib/src/transform/script_compactor.dart:52: // TODO(sigmund): should we > detect/remove duplicates? > On 2013/08/21 20:35:42, Siggi Cherem (dart-lang) wrote: > > On 2013/08/19 22:57:26, John Messerly wrote: > > > i think so. don't want to generate erroneous code that imports the same > > library > > > twice. however, we probably should run the main twice. > > > > > > Try running foo: > > > <script src="foo.dart"></script> > > > Try running foo again: > > > <script src="foo.dart"></script> > > > > > > foo.dart: > > > main() => print('FOO!'); > > > > But then, wouldn't it be ok to import it twice? (we anyways import them with a > > prefix) > > good point, importing twice would be fine assuming it doesn't trigger a Dart > language level error. if they are imported with unique prefixes that should work > great :) > > https://chromiumcodereview.appspot.com/22825012/diff/2001/lib/src/transform/s... > lib/src/transform/script_compactor.dart:119: > currentMirrorSystem().findLibrary(const Symbol('app_bootstrap')) > On 2013/08/21 20:35:42, Siggi Cherem (dart-lang) wrote: > > On 2013/08/19 22:57:26, John Messerly wrote: > > > seems unfortunate to need to use mirrors for this. couldn't we get the > HTML's > > > base URL? > > > > good question. I opened a bug to investigate. > > btw, I discovered another pattern using mirrors that is slightly simpler > (doesn't depend on findLibrary): > > currentMirrorSystem().isolate.rootLibrary.uri cool, I didn't know about rootLibrary! thx |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
