Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22396004:
Make observable transform a barback transform. (Closed)
|
Created:
7 years, 4 months ago by Siggi Cherem (dart-lang) Modified:
7 years, 4 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMake observable transform a barback transform.
R=jmesserly@google.com, nweiz@google.com, rnystrom@google.com
Committed: https://code.google.com/p/dart/source/detail?r=25985
Patch Set 1 #Patch Set 2 : changes alone (patchset 1 has the existing code, in the new location) #
Total comments: 58
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : review comments #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 31
Patch Set 9 : addressing john's comments #
Total comments: 4
Patch Set 10 : #
Messages
Total messages: 20 (0 generated)
The change is smaller than it looks. I moved some code from the polymer repo here (printer, transform, refactor, and their tests). To make the review easier, I created a patch that has the moved code without modifications. If you use the diff with respect to this patchset, you'll see what changed on those files. Hope that helps!
Adding Nathan. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.da... pkg/barback/lib/barback.dart:12: export 'src/transform.dart' show Transform, Log; Maybe "Logger"? https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:49: Log get log => _log; Document: "A logger so that the [Transformer] can report build details." https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:72: final Log _log = new Log(true); Add a TODO to create a separate one for each Transform. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:74: /// Log object used to report warnings and errors encountered while running a Add a TODO to move this into its own file. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:82: /// Logs a warning message. If present, [span] indicates the location in an "an asset that generated" -> "the input asset that caused" https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:89: /// asset that generated the error. Ditto above change here. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:97: var sb = new StringBuffer()..write(prefix)..write(' '); sb -> buffer https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:36: // transform. You could always do this fast check first and then only do a real parse if it passes to filter out false positives like "@observable" in a comment. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:47: if (transaction.hasEdits) { I would do: if (!transaction.hasEdits) return; To get rid of a level of nesting. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:56: }); Nice! https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:60: void _transformCompilationUnit( Should this be void? https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:105: final errors = new List<AnalysisError>(); final errors = <AnalysisError>[]; https://codereview.chromium.org/22396004/diff/3001/pkg/observe/test/transform... File pkg/observe/test/transform_test.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/test/transform... pkg/observe/test/transform_test.dart:73: throw "unsupported"; Maybe fail() here since it's a test failure if this is reached?
https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.da... pkg/barback/lib/barback.dart:12: export 'src/transform.dart' show Transform, Log; On 2013/08/08 22:47:09, Bob Nystrom wrote: > Maybe "Logger"? could be. But since that's currently the name in the logging package, I wonder if we want to preemptively prevent naming conflicts. Do we expect to use them for different purposes or do we expect both to converge? I could also add [span] argument there and use logging.Logger here. Thoughts? Maybe easier to chat in person? https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:49: Log get log => _log; On 2013/08/08 22:47:09, Bob Nystrom wrote: > Document: > > "A logger so that the [Transformer] can report build details." Done. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:72: final Log _log = new Log(true); On 2013/08/08 22:47:09, Bob Nystrom wrote: > Add a TODO to create a separate one for each Transform. Done. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:74: /// Log object used to report warnings and errors encountered while running a On 2013/08/08 22:47:09, Bob Nystrom wrote: > Add a TODO to move this into its own file. Done. I'm happy to move it out too (if we know what we want to do.) https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:82: /// Logs a warning message. If present, [span] indicates the location in an On 2013/08/08 22:47:09, Bob Nystrom wrote: > "an asset that generated" -> "the input asset that caused" Done. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:89: /// asset that generated the error. On 2013/08/08 22:47:09, Bob Nystrom wrote: > Ditto above change here. Done. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:97: var sb = new StringBuffer()..write(prefix)..write(' '); On 2013/08/08 22:47:09, Bob Nystrom wrote: > sb -> buffer Done. We all have different preferences on this one =) https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:47: if (transaction.hasEdits) { On 2013/08/08 22:47:09, Bob Nystrom wrote: > I would do: > > if (!transaction.hasEdits) return; > > To get rid of a level of nesting. Done. Yeah, I like doing that too. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:56: }); On 2013/08/08 22:47:09, Bob Nystrom wrote: > Nice! :) https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:60: void _transformCompilationUnit( On 2013/08/08 22:47:09, Bob Nystrom wrote: > Should this be void? oops, good catch, thanks. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:105: final errors = new List<AnalysisError>(); On 2013/08/08 22:47:09, Bob Nystrom wrote: > final errors = <AnalysisError>[]; Done. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/test/transform... File pkg/observe/test/transform_test.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/test/transform... pkg/observe/test/transform_test.dart:73: throw "unsupported"; On 2013/08/08 22:47:09, Bob Nystrom wrote: > Maybe fail() here since it's a test failure if this is reached? Done.
oops missed to reply to one comment. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:36: // transform. On 2013/08/08 22:47:09, Bob Nystrom wrote: > You could always do this fast check first and then only do a real parse if it > passes to filter out false positives like "@observable" in a comment. true, but I feel is ok in that case to do the work during apply.
https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.da... pkg/barback/lib/barback.dart:12: export 'src/transform.dart' show Transform, Log; On 2013/08/08 23:10:41, Siggi Cherem (dart-lang) wrote: > On 2013/08/08 22:47:09, Bob Nystrom wrote: > > Maybe "Logger"? > > could be. But since that's currently the name in the logging package, I wonder > if we want to preemptively prevent naming conflicts. > > Do we expect to use them for different purposes or do we expect both to > converge? I could also add [span] argument there and use logging.Logger here. > > Thoughts? Maybe easier to chat in person? I don't like the idea of barback exposing either "Log" or "Logger". Both of those are very likely to cause name conflicts. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:8: import 'package:source_maps/span.dart' show Span; Style nit: blank line between core library imports and package imports. Also, we usually don't use "show" unless there's a concrete problem with the default names that are exposed by a library. If we're using the source_maps package, it should be added to the pubspec. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:74: /// Log object used to report warnings and errors encountered while running a On 2013/08/08 22:47:09, Bob Nystrom wrote: > Add a TODO to move this into its own file. Or just move it now. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:82: /// Logs a warning message. If present, [span] indicates the location in an Style nit: the first paragraph of a doc comment should only be one sentence long. Also below. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:90: void error(String message, [Span span]) { Add a TODO to clarify when an error message should be logged vs thrown as an error. Eventually we'll probably allow a transformer to report multiple exceptions from a single run, which will muddy the waters even further. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:103: print(sb.toString()); Is it worth the extra complexity here to use a StringBuffer? https://codereview.chromium.org/22396004/diff/3001/pkg/codegen/lib/codegen.dart File pkg/codegen/lib/codegen.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/codegen/lib/codegen.da... pkg/codegen/lib/codegen.dart:1: library codegen; Copyright notice. https://codereview.chromium.org/22396004/diff/3001/pkg/codegen/pubspec.yaml File pkg/codegen/pubspec.yaml (right): https://codereview.chromium.org/22396004/diff/3001/pkg/codegen/pubspec.yaml#n... pkg/codegen/pubspec.yaml:5: transformations. Such as code printers and in-place edits of source files. "transformations. Such as" -> "transformations, such as" https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:36: // transform. On 2013/08/08 22:47:09, Bob Nystrom wrote: > You could always do this fast check first and then only do a real parse if it > passes to filter out false positives like "@observable" in a comment. There's no harm in running the transform on a file without any observable annotations, though, is there? It should just spit out the original file. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:44: var sourceFile = new SourceFile.text(id.toString(), content); Right now, [id.toString] will return a string of the form "package_name|path/to/file.dart". This is a representation we've been using for convenience internally, but it's not going to be understood well by code expecting normal paths or URIs, nor is it guaranteed to remain in that format in the future. It's probably safer to just pass [id.path] for now. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:47: if (transaction.hasEdits) { On 2013/08/08 22:47:09, Bob Nystrom wrote: > I would do: > > if (!transaction.hasEdits) return; > > To get rid of a level of nesting. If this returns without any outputs, that's not the same as passing the file through unmodified. You'll need to do that explicitly if that's your desired effect. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:54: new Asset.fromString(id.addExtension('.map'), printer.map)); This sourcemap is not going to consistently point to something real. Even if you pass [id.path] to [new SourceFile.text], that path is only going to be valid at the root level of whatever package this transform is being applied to. What's more, it will be overwritten by this transform; once you transform an asset, the previous version is no longer available. The only way this would work is if you made the sourcemap point to a file URI that identifies the original file on disk. However, barback doesn't expose that, and there can be cases where this transform is pipelined and that file doesn't exist anywhere at all on disk.
thanks Nathan, PTAL https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/barback.da... pkg/barback/lib/barback.dart:12: export 'src/transform.dart' show Transform, Log; On 2013/08/08 23:34:57, nweiz wrote: > On 2013/08/08 23:10:41, Siggi Cherem (dart-lang) wrote: > > On 2013/08/08 22:47:09, Bob Nystrom wrote: > > > Maybe "Logger"? > > > > could be. But since that's currently the name in the logging package, I wonder > > if we want to preemptively prevent naming conflicts. > > > > Do we expect to use them for different purposes or do we expect both to > > converge? I could also add [span] argument there and use logging.Logger here. > > > > Thoughts? Maybe easier to chat in person? > > I don't like the idea of barback exposing either "Log" or "Logger". Both of > those are very likely to cause name conflicts. renamed to TransformLogger https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:8: import 'package:source_maps/span.dart' show Span; On 2013/08/08 23:34:57, nweiz wrote: > Style nit: blank line between core library imports and package imports. Also, we > usually don't use "show" unless there's a concrete problem with the default > names that are exposed by a library. > > If we're using the source_maps package, it should be added to the pubspec. Done. Funny, I usually like using show if the list is short =), it makes it more explicit what your dependencies are and makes it easier to understand where does a type come from. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:74: /// Log object used to report warnings and errors encountered while running a On 2013/08/08 23:34:57, nweiz wrote: > On 2013/08/08 22:47:09, Bob Nystrom wrote: > > Add a TODO to move this into its own file. > > Or just move it now. now that it's just TransformLogger, it feels like part of this library. Do you still want me to put it separate? transform_logger.dart? https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:82: /// Logs a warning message. If present, [span] indicates the location in an On 2013/08/08 23:34:57, nweiz wrote: > Style nit: the first paragraph of a doc comment should only be one sentence > long. Also below. Done. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:90: void error(String message, [Span span]) { On 2013/08/08 23:34:57, nweiz wrote: > Add a TODO to clarify when an error message should be logged vs thrown as an > error. Eventually we'll probably allow a transformer to report multiple > exceptions from a single run, which will muddy the waters even further. Done. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:103: print(sb.toString()); On 2013/08/08 23:34:57, nweiz wrote: > Is it worth the extra complexity here to use a StringBuffer? good point. I used to have color printing and remvoed it to make things simpler, at which point the SB it's overkill. Done. https://codereview.chromium.org/22396004/diff/3001/pkg/codegen/lib/codegen.dart File pkg/codegen/lib/codegen.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/codegen/lib/codegen.da... pkg/codegen/lib/codegen.dart:1: library codegen; On 2013/08/08 23:34:57, nweiz wrote: > Copyright notice. Done. https://codereview.chromium.org/22396004/diff/3001/pkg/codegen/pubspec.yaml File pkg/codegen/pubspec.yaml (right): https://codereview.chromium.org/22396004/diff/3001/pkg/codegen/pubspec.yaml#n... pkg/codegen/pubspec.yaml:5: transformations. Such as code printers and in-place edits of source files. On 2013/08/08 23:34:57, nweiz wrote: > "transformations. Such as" -> "transformations, such as" Done. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:36: // transform. On 2013/08/08 23:34:57, nweiz wrote: > On 2013/08/08 22:47:09, Bob Nystrom wrote: > > You could always do this fast check first and then only do a real parse if it > > passes to filter out false positives like "@observable" in a comment. > > There's no harm in running the transform on a file without any observable > annotations, though, is there? It should just spit out the original file. correct. Or in my case not spit anything =) https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:44: var sourceFile = new SourceFile.text(id.toString(), content); On 2013/08/08 23:34:57, nweiz wrote: > Right now, [id.toString] will return a string of the form > "package_name|path/to/file.dart". This is a representation we've been using for > convenience internally, but it's not going to be understood well by code > expecting normal paths or URIs, nor is it guaranteed to remain in that format in > the future. It's probably safer to just pass [id.path] for now. I see. since the path doesn't have the package context, I would like this file url to have some additional information. I changed this to reconstruct the package: url if possible (if the path starts with lib/), otherwise to use id.path. Would it be possible for barback to expose a file url that works from the root of the project? https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:47: if (transaction.hasEdits) { On 2013/08/08 23:34:57, nweiz wrote: > On 2013/08/08 22:47:09, Bob Nystrom wrote: > > I would do: > > > > if (!transaction.hasEdits) return; > > > > To get rid of a level of nesting. > > If this returns without any outputs, that's not the same as passing the file > through unmodified. You'll need to do that explicitly if that's your desired > effect. I'm not sure I see what's the difference, so I'll drop shortly by to ask you about it. Basically what I want to indicate is that this asset shouldn't have been considered a primary. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:54: new Asset.fromString(id.addExtension('.map'), printer.map)); On 2013/08/08 23:34:57, nweiz wrote: > This sourcemap is not going to consistently point to something real. Even if you > pass [id.path] to [new SourceFile.text], that path is only going to be valid at > the root level of whatever package this transform is being applied to. What's > more, it will be overwritten by this transform; once you transform an asset, the > previous version is no longer available. > > The only way this would work is if you made the sourcemap point to a file URI > that identifies the original file on disk. However, barback doesn't expose that, > and there can be cases where this transform is pipelined and that file doesn't > exist anywhere at all on disk. Oh my! those are good points. I don't know yet what's the best solution. I created this bug so we can brainstorm and track progress: https://dartbug.com/12339
https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:47: if (transaction.hasEdits) { On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) wrote: > On 2013/08/08 23:34:57, nweiz wrote: > > On 2013/08/08 22:47:09, Bob Nystrom wrote: > > > I would do: > > > > > > if (!transaction.hasEdits) return; > > > > > > To get rid of a level of nesting. > > > > If this returns without any outputs, that's not the same as passing the file > > through unmodified. You'll need to do that explicitly if that's your desired > > effect. > > I'm not sure I see what's the difference, so I'll drop shortly by to ask you > about it. > > Basically what I want to indicate is that this asset shouldn't have been > considered a primary. Fixed. So it turns out that if we dont emit it, it gets 'consumed' and a future phase of transformers wont see it. (Thanks for the clarification!)
barback changes lgtm, with a couple more suggestions https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:74: /// Log object used to report warnings and errors encountered while running a On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) wrote: > On 2013/08/08 23:34:57, nweiz wrote: > > On 2013/08/08 22:47:09, Bob Nystrom wrote: > > > Add a TODO to move this into its own file. > > > > Or just move it now. > > now that it's just TransformLogger, it feels like part of this library. Do you > still want me to put it separate? transform_logger.dart? I'd still prefer it to be separate. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:44: var sourceFile = new SourceFile.text(id.toString(), content); On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) wrote: > On 2013/08/08 23:34:57, nweiz wrote: > > Right now, [id.toString] will return a string of the form > > "package_name|path/to/file.dart". This is a representation we've been using > for > > convenience internally, but it's not going to be understood well by code > > expecting normal paths or URIs, nor is it guaranteed to remain in that format > in > > the future. It's probably safer to just pass [id.path] for now. > > I see. since the path doesn't have the package context, I would like this file > url to have some additional information. I changed this to reconstruct the > package: url if possible (if the path starts with lib/), otherwise to use > id.path. > > Would it be possible for barback to expose a file url that works from the root > of the project? Not in general. Intermediate assets may have no on-disk representation, and thus no valid file URL. Right now, pub's implementation of PackageProvider ensures that all source assets will come from a package's lib/ or asset/ directory. If we wanted to encode that more directly into barback, it would be possible to always be able to generate a "package:" or "asset:" URL for a given asset. That said, the assets those URLs referred to could be an intermediate version of an asset that gets transformed multiple times, and may thus not be outwardly accessible in the form this transformer sees them. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:54: new Asset.fromString(id.addExtension('.map'), printer.map)); On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) wrote: > On 2013/08/08 23:34:57, nweiz wrote: > > This sourcemap is not going to consistently point to something real. Even if > you > > pass [id.path] to [new SourceFile.text], that path is only going to be valid > at > > the root level of whatever package this transform is being applied to. What's > > more, it will be overwritten by this transform; once you transform an asset, > the > > previous version is no longer available. > > > > The only way this would work is if you made the sourcemap point to a file URI > > that identifies the original file on disk. However, barback doesn't expose > that, > > and there can be cases where this transform is pipelined and that file doesn't > > exist anywhere at all on disk. > > Oh my! those are good points. I don't know yet what's the best solution. I > created this bug so we can brainstorm and track progress: > https://dartbug.com/12339 I'd just get rid of the sourcemaps here for now until we get better barback support for it.
thanks! https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:74: /// Log object used to report warnings and errors encountered while running a On 2013/08/09 00:53:48, nweiz wrote: > On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) wrote: > > On 2013/08/08 23:34:57, nweiz wrote: > > > On 2013/08/08 22:47:09, Bob Nystrom wrote: > > > > Add a TODO to move this into its own file. > > > > > > Or just move it now. > > > > now that it's just TransformLogger, it feels like part of this library. Do you > > still want me to put it separate? transform_logger.dart? > > I'd still prefer it to be separate. Done. https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/observe/lib/transform.... pkg/observe/lib/transform.dart:54: new Asset.fromString(id.addExtension('.map'), printer.map)); On 2013/08/09 00:53:48, nweiz wrote: > On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) wrote: > > On 2013/08/08 23:34:57, nweiz wrote: > > > This sourcemap is not going to consistently point to something real. Even if > > you > > > pass [id.path] to [new SourceFile.text], that path is only going to be valid > > at > > > the root level of whatever package this transform is being applied to. > What's > > > more, it will be overwritten by this transform; once you transform an asset, > > the > > > previous version is no longer available. > > > > > > The only way this would work is if you made the sourcemap point to a file > URI > > > that identifies the original file on disk. However, barback doesn't expose > > that, > > > and there can be cases where this transform is pipelined and that file > doesn't > > > exist anywhere at all on disk. > > > > Oh my! those are good points. I don't know yet what's the best solution. I > > created this bug so we can brainstorm and track progress: > > https://dartbug.com/12339 > > I'd just get rid of the sourcemaps here for now until we get better barback > support for it. ok, no prob. I removed it for now. Our current use case only requires one transform on .dart files, so it might be ok to add it back before barback has full support for it.
https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:8: import 'package:source_maps/span.dart' show Span; On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) wrote: > On 2013/08/08 23:34:57, nweiz wrote: > > Style nit: blank line between core library imports and package imports. Also, > we > > usually don't use "show" unless there's a concrete problem with the default > > names that are exposed by a library. > > > > If we're using the source_maps package, it should be added to the pubspec. > > Done. > > Funny, I usually like using show if the list is short =), it makes it more > explicit what your dependencies are and makes it easier to understand where does > a type come from. Moreover, it's the only style (along with "as") which reliably avoids name conflicts due to new top-level members introduced. It is likely tools will start doing this automatically and it will be generally encouraged style. There have been various threads on this topic. It's not ideal, but it's probably what we're stuck with :| https://codereview.chromium.org/22396004/diff/12003/pkg/barback/pubspec.yaml File pkg/barback/pubspec.yaml (right): https://codereview.chromium.org/22396004/diff/12003/pkg/barback/pubspec.yaml#... pkg/barback/pubspec.yaml:17: source_maps: any not sure if this was supposed to be sorted ... but it isn't anymore :) https://codereview.chromium.org/22396004/diff/12003/pkg/codegen/lib/codegen.dart File pkg/codegen/lib/codegen.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/codegen/lib/codegen.d... pkg/codegen/lib/codegen.dart:5: library codegen; IMHO -- this is way too generic of a name given the current content of this package. Since there isn't much here, could we merge this into another pkg? Could they plausibly be helpers inside source_maps? Or analyzer, since it's concerned with analysis and code transformation (refactorings etc)? https://codereview.chromium.org/22396004/diff/12003/pkg/codegen/lib/refactor.... File pkg/codegen/lib/refactor.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/codegen/lib/refactor.... pkg/codegen/lib/refactor.dart:1: // for details. All rights reserved. Use of this source code is governed by a first line of copyright disappeared? https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; sanity check: barback doesn't depend on "dart:io" right? https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:32: // Note: technically we should parse the file to find accurately the is there no way to save state between this check and the actual application? that seems unfortunate. In web_ui we cache the parsed AST so we can avoid expensive reparsing... https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:37: return input.readAsString().then((c) => c.contains("@observable")); a parse should be pretty fast, right? I wonder if the overhead of a correct implementation even matters. it might be that the file IO dominates, in which case this heuristic is nearly as expensive as an accurate check. Just a hypothesis tho, needs to be measured. https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:41: return transform.primaryInput it's a small detail, but it seems goofy that a transform is passed in which requires an immediate ".then" to do anything useful with... and then you need to do another async operation to get its text... https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:55: // TODO(sigmund): emit source maps when backback supports it (see barback? :) https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:56: // dartbug.com/12340) idea: add a dummy "addSourceMap" method so you can pass it, but leave it to barback folks to implement the method? https://codereview.chromium.org/22396004/diff/12003/pkg/observe/test/transfor... File pkg/observe/test/transform_test.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/test/transfor... pkg/observe/test/transform_test.dart:14: group('fixes contructor calls ', () { idea: revive the test that checks we inserted the mixin? Except instead of inserting the mixin, it tests that we replaced Observable with ChangeNotifier.
Some more comments, but barback changes LGTM. https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... File pkg/barback/lib/src/transform.dart (right): https://codereview.chromium.org/22396004/diff/3001/pkg/barback/lib/src/transf... pkg/barback/lib/src/transform.dart:8: import 'package:source_maps/span.dart' show Span; On 2013/08/09 05:44:03, John Messerly wrote: > On 2013/08/09 00:29:54, Siggi Cherem (dart-lang) wrote: > > On 2013/08/08 23:34:57, nweiz wrote: > > > Style nit: blank line between core library imports and package imports. > Also, > > we > > > usually don't use "show" unless there's a concrete problem with the default > > > names that are exposed by a library. > > > > > > If we're using the source_maps package, it should be added to the pubspec. > > > > Done. > > > > Funny, I usually like using show if the list is short =), it makes it more > > explicit what your dependencies are and makes it easier to understand where > does > > a type come from. > > Moreover, it's the only style (along with "as") which reliably avoids name > conflicts due to new top-level members introduced. Since this is a "package:" import, that doesn't worry me. New top-level names will only appear when a developer upgrades the version of source_maps explicitly and they can resolve it then. > It is likely tools will start doing this automatically and it will be generally encouraged style. If tools are going to do it for us (at deploy time), all the more reason to not do it manually. :) https://codereview.chromium.org/22396004/diff/12003/pkg/barback/pubspec.yaml File pkg/barback/pubspec.yaml (right): https://codereview.chromium.org/22396004/diff/12003/pkg/barback/pubspec.yaml#... pkg/barback/pubspec.yaml:17: source_maps: any On 2013/08/09 05:44:03, John Messerly wrote: > not sure if this was supposed to be sorted ... but it isn't anymore :) Sorted is good. :) https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 05:44:03, John Messerly wrote: > sanity check: barback doesn't depend on "dart:io" right? Yes, barback does. That means this library in observe is coupled to dart:io, but other libraries in here that don't import transform.dart aren't. Is that OK? https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:41: return transform.primaryInput On 2013/08/09 05:44:03, John Messerly wrote: > it's a small detail, but it seems goofy that a transform is passed in which > requires an immediate ".then" to do anything useful with... and then you need to > do another async operation to get its text... We could probably make accessing the primary input sync, but accessing other inputs will always be async, so that may not buy you much. Once you have an asset, reading its contents are async. Async is a pain, but I want to make sure transformers are as performant as possible so I don't want one to block all of barback on a sync IO call.
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:32: // Note: technically we should parse the file to find accurately the On 2013/08/09 05:44:03, John Messerly wrote: > is there no way to save state between this check and the actual application? > that seems unfortunate. > In web_ui we cache the parsed AST so we can avoid expensive reparsing... It's fine if [isPrimary] generates some false positives -- [apply] can always just do nothing if nothing needs to be done. It's more helpful to barback if [isPrimary] generates false positives quickly than if it's slow but completely accurate. https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:41: return transform.primaryInput On 2013/08/09 15:42:38, Bob Nystrom wrote: > On 2013/08/09 05:44:03, John Messerly wrote: > > it's a small detail, but it seems goofy that a transform is passed in which > > requires an immediate ".then" to do anything useful with... and then you need > to > > do another async operation to get its text... > > We could probably make accessing the primary input sync, but accessing other > inputs will always be async, so that may not buy you much. Once you have an > asset, reading its contents are async. Async is a pain, but I want to make sure > transformers are as performant as possible so I don't want one to block all of > barback on a sync IO call. The trouble with making [primaryInput] sync is that it could become dirty before it's accessed by [apply]. I suppose we could just throw an exception there, though.
Thanks guys. Replied to a few comments. Main thing left to review are the changes to move the printer and transaction into the source_maps package. Nothing else changed in the barback side of things. https://codereview.chromium.org/22396004/diff/12003/pkg/barback/pubspec.yaml File pkg/barback/pubspec.yaml (right): https://codereview.chromium.org/22396004/diff/12003/pkg/barback/pubspec.yaml#... pkg/barback/pubspec.yaml:17: source_maps: any On 2013/08/09 15:42:38, Bob Nystrom wrote: > On 2013/08/09 05:44:03, John Messerly wrote: > > not sure if this was supposed to be sorted ... but it isn't anymore :) > > Sorted is good. :) Done. https://codereview.chromium.org/22396004/diff/12003/pkg/codegen/lib/codegen.dart File pkg/codegen/lib/codegen.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/codegen/lib/codegen.d... pkg/codegen/lib/codegen.dart:5: library codegen; On 2013/08/09 05:44:03, John Messerly wrote: > IMHO -- this is way too generic of a name given the current content of this > package. > > Since there isn't much here, could we merge this into another pkg? > Could they plausibly be helpers inside source_maps? Or analyzer, since it's > concerned with analysis and code transformation (refactorings etc)? Ok - refactored things a bit. These two helpers moved to source_maps. but code-printer is now more generic (no 'Declarations'). I moved the tests there too and added a couple tests for NestedPrinter. We might want to rename 'source_maps' though. Ideas welcomed :-) https://codereview.chromium.org/22396004/diff/12003/pkg/codegen/lib/refactor.... File pkg/codegen/lib/refactor.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/codegen/lib/refactor.... pkg/codegen/lib/refactor.dart:1: // for details. All rights reserved. Use of this source code is governed by a On 2013/08/09 05:44:03, John Messerly wrote: > first line of copyright disappeared? oops, fixed. https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 15:42:38, Bob Nystrom wrote: > On 2013/08/09 05:44:03, John Messerly wrote: > > sanity check: barback doesn't depend on "dart:io" right? > > Yes, barback does. That means this library in observe is coupled to dart:io, but > other libraries in here that don't import transform.dart aren't. Is that OK? For now yes, but I think eventually it would be nice to decouple barback a bit. At least so that if I implement a transform I don't have to depend on it. https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:32: // Note: technically we should parse the file to find accurately the On 2013/08/09 05:44:03, John Messerly wrote: > is there no way to save state between this check and the actual application? > that seems unfortunate. > In web_ui we cache the parsed AST so we can avoid expensive reparsing... In terms of caching - I thought about doing that too. My understanding is that there is not a guarantee that this class will be called twice (apply could be called on a new instance of this class). Talking yesterday with Bob and Nathan, I got the expectation that isPrimary and apply might get merged in a single function? that might help in this case. https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:37: return input.readAsString().then((c) => c.contains("@observable")); On 2013/08/09 05:44:03, John Messerly wrote: > a parse should be pretty fast, right? I wonder if the overhead of a correct > implementation even matters. it might be that the file IO dominates, in which > case this heuristic is nearly as expensive as an accurate check. Just a > hypothesis tho, needs to be measured. Not sure. I'm currently inclined to leave it as is (or even run on all .dart files and remove this check alltogether), and optimize it later. https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:41: return transform.primaryInput On 2013/08/09 19:41:26, nweiz wrote: > On 2013/08/09 15:42:38, Bob Nystrom wrote: > > On 2013/08/09 05:44:03, John Messerly wrote: > > > it's a small detail, but it seems goofy that a transform is passed in which > > > requires an immediate ".then" to do anything useful with... and then you > need > > to > > > do another async operation to get its text... > > > > We could probably make accessing the primary input sync, but accessing other > > inputs will always be async, so that may not buy you much. Once you have an > > asset, reading its contents are async. Async is a pain, but I want to make > sure > > transformers are as performant as possible so I don't want one to block all of > > barback on a sync IO call. > > The trouble with making [primaryInput] sync is that it could become dirty before > it's accessed by [apply]. I suppose we could just throw an exception there, > though. From yesterday's discussion I thought that you guaranteed that from the moment you called isPrimary until you call apply the asset would not change. Did I misunderstand? https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:55: // TODO(sigmund): emit source maps when backback supports it (see On 2013/08/09 05:44:03, John Messerly wrote: > barback? :) =) https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:56: // dartbug.com/12340) On 2013/08/09 05:44:03, John Messerly wrote: > idea: add a dummy "addSourceMap" method so you can pass it, but leave it to > barback folks to implement the method? I posted a comment about this in the bug, for now I'll keep it separate from this CL. https://codereview.chromium.org/22396004/diff/12003/pkg/observe/test/transfor... File pkg/observe/test/transform_test.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/test/transfor... pkg/observe/test/transform_test.dart:14: group('fixes contructor calls ', () { On 2013/08/09 05:44:03, John Messerly wrote: > idea: revive the test that checks we inserted the mixin? > > Except instead of inserting the mixin, it tests that we replaced Observable with > ChangeNotifier. Done. https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/print... File pkg/source_maps/lib/printer.dart (right): https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/print... pkg/source_maps/lib/printer.dart:91: /// A more advanced printer that keeps track of offset locations to record this code is very similar to the original main differences here are: - switched /** to /// to be consistent with the convention of this package - renamed CodePrinter to NestedPrinter - introduced NestedItem below - got rid of Declarations (too specific about generating Dart code) - adjusted comments to use NestedItem instead of 'CodePrinter' and 'Declarations' https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/test/refa... File pkg/source_maps/test/refactor_test.dart (right): https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/test/refa... pkg/source_maps/test/refactor_test.dart:46: }); Above this line is the same tests as before (changed only to put them in a group). The test below is new:
lgtm https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/refac... File pkg/source_maps/lib/refactor.dart (right): https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/refac... pkg/source_maps/lib/refactor.dart:103: /** triple slash?
thx! https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/refac... File pkg/source_maps/lib/refactor.dart (right): https://codereview.chromium.org/22396004/diff/72001/pkg/source_maps/lib/refac... pkg/source_maps/lib/refactor.dart:103: /** On 2013/08/09 22:14:47, John Messerly wrote: > triple slash? I had a feeling I was going to miss one... thanks!
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 22:09:09, Siggi Cherem (dart-lang) wrote: > On 2013/08/09 15:42:38, Bob Nystrom wrote: > > On 2013/08/09 05:44:03, John Messerly wrote: > > > sanity check: barback doesn't depend on "dart:io" right? > > > > Yes, barback does. That means this library in observe is coupled to dart:io, > but > > other libraries in here that don't import transform.dart aren't. Is that OK? > > For now yes, but I think eventually it would be nice to decouple barback a bit. > At least so that if I implement a transform I don't have to depend on it. I don't think it makes sense either for barback not to depend on dart:io (file-based Assets are fundamentally tied to the filesystem, and we may put caching support in barback at some point). It's easy for a package to have a transform that depends on dart:io without having the rest of the package's libraries do so, so I think that's likely to be what we'll do in the future. https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:32: // Note: technically we should parse the file to find accurately the On 2013/08/09 22:09:09, Siggi Cherem (dart-lang) wrote: > On 2013/08/09 05:44:03, John Messerly wrote: > > is there no way to save state between this check and the actual application? > > that seems unfortunate. > > In web_ui we cache the parsed AST so we can avoid expensive reparsing... > > In terms of caching - I thought about doing that too. My understanding is that > there is not a guarantee that this class will be called twice (apply could be > called on a new instance of this class). > > Talking yesterday with Bob and Nathan, I got the expectation that isPrimary and > apply might get merged in a single function? that might help in this case. I don't think it's likely that we'll merge the two methods. If that's a pattern that's easiest for your application, it's already available by just having a very liberal implementation of [isPrimary] (you could even have it always return true). https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:41: return transform.primaryInput On 2013/08/09 22:09:09, Siggi Cherem (dart-lang) wrote: > On 2013/08/09 19:41:26, nweiz wrote: > > On 2013/08/09 15:42:38, Bob Nystrom wrote: > > > On 2013/08/09 05:44:03, John Messerly wrote: > > > > it's a small detail, but it seems goofy that a transform is passed in > which > > > > requires an immediate ".then" to do anything useful with... and then you > > need > > > to > > > > do another async operation to get its text... > > > > > > We could probably make accessing the primary input sync, but accessing other > > > inputs will always be async, so that may not buy you much. Once you have an > > > asset, reading its contents are async. Async is a pain, but I want to make > > sure > > > transformers are as performant as possible so I don't want one to block all > of > > > barback on a sync IO call. > > > > The trouble with making [primaryInput] sync is that it could become dirty > before > > it's accessed by [apply]. I suppose we could just throw an exception there, > > though. > > From yesterday's discussion I thought that you guaranteed that from the moment > you called isPrimary until you call apply the asset would not change. Did I > misunderstand? We can't guarantee that; the asset can change on disk and there's nothing barback can do to prevent that. It's just a question of how we expose that change to the transformer. We'll end up re-running the transformer anyway, so it's not unreasonable to say that it's just an error, but right now what happens is that [primaryInput] will return the new value.
Message was sent while issue was closed.
Committed patchset #10 manually as r25985 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 22:21:28, nweiz wrote: > On 2013/08/09 22:09:09, Siggi Cherem (dart-lang) wrote: > > On 2013/08/09 15:42:38, Bob Nystrom wrote: > > > On 2013/08/09 05:44:03, John Messerly wrote: > > > > sanity check: barback doesn't depend on "dart:io" right? > > > > > > Yes, barback does. That means this library in observe is coupled to dart:io, > > but > > > other libraries in here that don't import transform.dart aren't. Is that OK? > > > > For now yes, but I think eventually it would be nice to decouple barback a > bit. > > At least so that if I implement a transform I don't have to depend on it. > > I don't think it makes sense either for barback not to depend on dart:io > (file-based Assets are fundamentally tied to the filesystem, and we may put > caching support in barback at some point). It's easy for a package to have a > transform that depends on dart:io without having the rest of the package's > libraries do so, so I think that's likely to be what we'll do in the future. I think it would be nice to be able to run transfomers also in other contexts (e.g. in a browser's web worker, for example). It's totally ok for barback to depend on dart:io, but I would be nice if it doesn't push this dependency on transfomers too. I think we can achieve that by simply splitting the Asset API (so the interface is separate from how to create file assets).
Message was sent while issue was closed.
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 22:38:33, Siggi Cherem (dart-lang) wrote: > On 2013/08/09 22:21:28, nweiz wrote: > > On 2013/08/09 22:09:09, Siggi Cherem (dart-lang) wrote: > > > On 2013/08/09 15:42:38, Bob Nystrom wrote: > > > > On 2013/08/09 05:44:03, John Messerly wrote: > > > > > sanity check: barback doesn't depend on "dart:io" right? > > > > > > > > Yes, barback does. That means this library in observe is coupled to > dart:io, > > > but > > > > other libraries in here that don't import transform.dart aren't. Is that > OK? > > > > > > For now yes, but I think eventually it would be nice to decouple barback a > > bit. > > > At least so that if I implement a transform I don't have to depend on it. > > > > I don't think it makes sense either for barback not to depend on dart:io > > (file-based Assets are fundamentally tied to the filesystem, and we may put > > caching support in barback at some point). It's easy for a package to have a > > transform that depends on dart:io without having the rest of the package's > > libraries do so, so I think that's likely to be what we'll do in the future. > > I think it would be nice to be able to run transfomers also in other contexts > (e.g. in a browser's web worker, for example). > > It's totally ok for barback to depend on dart:io, but I would be nice if it > doesn't push this dependency on transfomers too. I think we can achieve that by > simply splitting the Asset API (so the interface is separate from how to create > file assets). +1 siggi
Message was sent while issue was closed.
https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... File pkg/observe/lib/transform.dart (right): https://codereview.chromium.org/22396004/diff/12003/pkg/observe/lib/transform... pkg/observe/lib/transform.dart:17: import 'package:barback/barback.dart'; On 2013/08/09 22:46:50, John Messerly wrote: > On 2013/08/09 22:38:33, Siggi Cherem (dart-lang) wrote: > > On 2013/08/09 22:21:28, nweiz wrote: > > > On 2013/08/09 22:09:09, Siggi Cherem (dart-lang) wrote: > > > > On 2013/08/09 15:42:38, Bob Nystrom wrote: > > > > > On 2013/08/09 05:44:03, John Messerly wrote: > > > > > > sanity check: barback doesn't depend on "dart:io" right? > > > > > > > > > > Yes, barback does. That means this library in observe is coupled to > > dart:io, > > > > but > > > > > other libraries in here that don't import transform.dart aren't. Is that > > OK? > > > > > > > > For now yes, but I think eventually it would be nice to decouple barback a > > > bit. > > > > At least so that if I implement a transform I don't have to depend on it. > > > > > > I don't think it makes sense either for barback not to depend on dart:io > > > (file-based Assets are fundamentally tied to the filesystem, and we may put > > > caching support in barback at some point). It's easy for a package to have a > > > transform that depends on dart:io without having the rest of the package's > > > libraries do so, so I think that's likely to be what we'll do in the future. > > > > I think it would be nice to be able to run transfomers also in other contexts > > (e.g. in a browser's web worker, for example). > > > > It's totally ok for barback to depend on dart:io, but I would be nice if it > > doesn't push this dependency on transfomers too. I think we can achieve that > by > > simply splitting the Asset API (so the interface is separate from how to > create > > file assets). > > +1 siggi Transformer is a class that wraps code that can do some sort of conceptual transformation in a form that barback can understand. If you want to use that transformation elsewhere, it's easy to factor out a library that exposes a programmatic API for it and have the barback Transformer call out to that library. I don't think it makes sense to change the barback API so it can be called in a non-barback context. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
