-
Notifications
You must be signed in to change notification settings - Fork 72
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
base: master
Are you sure you want to change the base?
Conversation
Oo, cool! 🎉 I had a similar idea on this --- except, for me, it was going to be part of #286 because By that same token, I definitely don't want it touching I can work with you in finalizing this, especially when it comes to extracting a But the general idea is that any That way, when it comes to 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 😆. |
Yeah, I figured this would go hand-in-hand with splitting off utils (had to copy I would argue that Should be fairly straightforward to make this only at the plugin level, with a standardized |
Right, To your point tho, I did consider extract
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 I realize that muddies a "util" vs a "plugin". To remedy, I think the logic should be mostly within the Thinking out loud here, sorry for ramble. 😆 |
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:
Usage
Build-in support:
Custom usage:
Plugin usage:
Discussion
1. Should this be included by default with the
taskr
packageI 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 thesourceMap
property is found on a file, it is written as an inline source map. ThesourceMap
property should only be set if source maps are enabled in.source
orinitSourceMaps
, 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!