Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add @taskr/sourcemaps #294

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

Add @taskr/sourcemaps #294

wants to merge 3 commits into from

Conversation

timhall
Copy link

@timhall timhall commented Aug 2, 2017

This PR adds source map init, write, and apply for use in tasks and plugins. The approach is very similar to Gulp 4 (in fact most of the code is based on gulp-sourcemaps). Important improvements include:

  • Maintains the source map chain through steps, ensuring that the final output maps back to the original input
  • Greatly simplifies source map handling for plugins

Usage

Build-in support:

// 1. Initialize / load existing source maps
// 2. Write inline source maps
yield task.source('**/*.js', { sourcemaps: true })
  .babel()
  .target('build');

// 1. Initialize / load existing source maps
// 2. Write external source maps (next to source files)
yield task.source('**/*.scss', { sourcemaps: true })
  .sass()
  .target('build', { sourcemaps: '.' });

Custom usage:

yield task.source('**/*.ts')
  .typescript()
  .initSourceMaps({ load: true })
  .babel()
  .writeSourceMaps({ dest: '.' })
  .gzip()
  .target('build');

Plugin usage:

const applySourceMap = require('@taskr/sourcemaps').applySourceMap;
const transform = require('./transform');

module.exports = function(task, options) {
  task.plugin('transform', { every: true }, function * (file) {
    // If sourceMap is enabled for file, make source maps
    if (file.sourceMap) {
      options.makeSourceMap = true;
    }

    const result = transform(file.data, options);
    file.data = result.code;

    // Apply source map, merging with existing
    if (file.sourceMap) {
      applySourceMap(file, result.map);
    }
  });
}

Discussion

1. Should this be included by default with the taskr package

I think building gulp-sourcemaps into gulp 4 was well received and would be a nice addition to taskr. Currently, most of the plugins have source map support currently, which is awesome, but they only support merging if the tool supports it (which is rare) and some don't allow control between inline and external source maps. This adds 4 direct dependencies, but they have no sub-dependencies, so in total it's fairly reasonable at 80k.

Alternative: It could potentially be an optional dependency that needs to be explicitly installed, but it will most likely be installed by plugins needing applySourceMap that in the end it may not lead to savings for the user to make it optional.

2. Should source maps be written by default

Currently, unless source map writing is explicitly disabled with .target('...', { sourcemaps: false }), if the sourceMap property is found on a file, it is written as an inline source map. The sourceMap property should only be set if source maps are enabled in .source or initSourceMaps, so it's reasonable to assume it is an intelligent default to write them.

I've added support to @taskr/babel, wanted to see what you think before I went any further. Thanks!

@lukeed lukeed added the future label Aug 2, 2017
@lukeed
Copy link
Owner

lukeed commented Aug 2, 2017

Oo, cool! 🎉

I had a similar idea on this --- except, for me, it was going to be part of #286 because sourcemapping is a utility feature.

By that same token, I definitely don't want it touching core at all. The reason being that Taskr is a task-runner... not a build tool. It can be used as one, but that's not its primary function.

I can work with you in finalizing this, especially when it comes to extracting a @utils package.

But the general idea is that any *.map file is just a file. Adding or altering the map is exactly the same process as adding or altering any other file. The actual sourceMap logic can be handled inside the package (or even offloaded to another module, like you have), but once that logic has been completed, it'll push its changes to the current task._.files array.

That way, when it comes to target() the output, the map's final-form is written to the destination file.

A grand total of 0 changes need to be made in core for that to happen. Nor any added dependencies!

Does that sound fair/reasonable to you?

As mentioned, happy to collaborate with you on this. FWIW I do think this is needed -- would have made things easier in writing a bunch of plugins 😆.

@timhall
Copy link
Author

timhall commented Aug 2, 2017

Yeah, I figured this would go hand-in-hand with splitting off utils (had to copy read in temporarily until that's done).

I would argue that source and target are file-specific and a source map is file metadata that makes sense to live with the file, but I can totally understand reducing API surface area in core.

Should be fairly straightforward to make this only at the plugin level, with a standardized sourcemaps option that is either true for inline or a path for external. applySourceMap would load and merge into an existing source map from inspecting file.data. Some potential tricky issues are filtering previous external maps from subsequent plugins (should only use inline until the last step if you want external) and ensuring the user enables sourcemaps for each step, but they're docs-type fixes.

@lukeed
Copy link
Owner

lukeed commented Aug 2, 2017

Right, source and target (potentially) mark the beginnings & ends of pipelines. What happens in between means nothing to them. A sourceMap is as irrelevant to them as a styles.css or a 10MB archive. What happens in between those two methods means nothing to the methods themselves.

To your point tho, I did consider extract source and target into "plugins" (leaving start, serial, parallel, and plugin as the only API) for philosophy consistency, but I figured that that may have felt too modular/segmented to the existing users, since 99% of use cases (even my own) rely on source.

This might still happen down the road.

I see plugin authors doing this:

const babel = require('babel');
const utils = require('@taskr/utils');

module.exports = function (task) {
  task.plugin('babel', {}, function * (file, opts) {
    // ...
    
    // both are one of: `true` | `both` | `inline`
    opts.sourceMap = opts.sourceMap || file.sourceMap;
    const isType = opts.sourceMap;

    const output = babel.transform(file.data, opts);

    if (opts.sourceMap && output.map) {
      utils.sourcemaps.call(task, file, output.map, isType);
    }
    

    // every call will update matching `file.base`.map
    // or update internal `file.data` content
  });
}

Names and such can be changed, of course. But the idea is that utils.sourcemaps handles all the writing & updating logic... just like any other plugin would for its own API.


I realize that muddies a "util" vs a "plugin". To remedy, I think the logic should be mostly within the utils.sourcemap & then the would-be plugin imports & wraps that with an official name & some methods.

Thinking out loud here, sorry for ramble. 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants