futurechan / gulp-asset-transform

13 stars 3 forks source link

Stream #6

Closed bjornarg closed 9 years ago

bjornarg commented 9 years ago

Stream option as discussed in PR #5

futurechan commented 9 years ago

Ok, I think stream is better than tasks. It allows for logic in addition to an ordered list of tasks. Do you see any reason to keep tasks other legacy support?

I think the name stream makes a lot of sense, but since stream and tasks are mutually exclusive, do you think using tasks to support a function, array of functions, or array of tasks would be appropriate? Or, if stream is preferred maybe we should do stream = tasks = stream.

Since you are the first person to show interest in this project, I'm using you as a sounding board. I hope that's ok.

futurechan commented 9 years ago

Perhaps a task should mean a gulp task or a function that returns a stream like a gulp task. From this, perhaps tasks could be an array of gulp tasks, a function that returns a stream like a gulp task, or potentially an array of functions that return streams like gulp tasks. Would this solve the task reuse problem from #5?

bjornarg commented 9 years ago

Well, these two pull requests are not mutually exclusive, they just provide two ways to basically achieve the same goal with slightly different syntax; your pipelines can be used more than once. I see no reason to remove the tasks syntax currently, as it would break any current usage of the plugin. Perhaps add a deprecated warning if you wish to remove it in a later version. If you wish to remove the tasks syntax then PR #5 isn't really useful, but if you're only using the pipeline once, perhaps the tasks syntax is easier. It's definately shorter.

futurechan commented 9 years ago

I see! I agree the tasks syntax is more concise. If we use a function like this:

var myPipeline = function (files, filename) {
    return files
        .pipe(uglify())
        .pipe(concat(filename))
        .pipe(rev());
}

Couldn't we assign that more than once?

at({
  id1: {
    stream: myPipeline
  },
  id2: {
    stream: myPipeline
  }
})

I agree we should issue a warning that the tasks array will be deprecated in a future version.

bjornarg commented 9 years ago

Oh, yes, that's not the issue. The problem was:

at({
  id1: { tasks: [task()] }
})
<!-- at:id1 >> lib.js -->
<script src="library.js"></script>
<!-- at:end -->
<!-- at:id1 >> app.js -->
<script src="app.js"></script>
<!-- at:end -->

When lib.js is created, the stream created by task() is bascially "consumed" (it's ended), but when app.js is run, it will attempt to use the same stream, but will fail.

The stream syntax from this PR wouldn't suffer from the same issue, that's just an issue with the current tasks syntax, because there's currently no way to create the stream when needed, it's just created when the gulp task is created. The stream syntax would provide a function that would create each task in the stream when called, so we'd basically create a new stream each time.

futurechan commented 9 years ago

Yes, I was wondering if the tasks array was really necessary with this PR. I've decided it is and the convenience of the tasks syntax merits being maintained.

I'm going to accept this and bump the version to 1.5. I will also be creating a 1.5 milestone an referencing this PR from #2 where I'll add a few more items to be done before a publish.

Thanks for all your work.