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

fix: improve comments handling in presets + api + gulp #855

Merged
merged 3 commits into from May 17, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -26,6 +26,7 @@ module.exports = function(t) {

function convert(nodes) {
const exprs = [];
const comments = [];

for (let i = 0; i < nodes.length; i++) {
const bail = () => {
Expand All @@ -47,6 +48,9 @@ module.exports = function(t) {
if (t.isExpression(node)) {
exprs.push(node);
} else if (t.isExpressionStatement(node)) {
if (node.leadingComments) {
comments.push(...node.leadingComments);
}
if (node.expression) exprs.push(node.expression);
} else if (t.isIfStatement(node)) {
let consequent;
Expand Down Expand Up @@ -97,6 +101,14 @@ module.exports = function(t) {
seq = t.sequenceExpression(exprs);
}

/**
* collect all the comment ast nodes that are before expression
* statments and add it to the new generated node
*/
if (seq) {
seq.leadingComments = comments;
}

/* eslint-disable no-self-assign */
seq = seq;
return { seq };
Expand Down
Expand Up @@ -4,4 +4,8 @@ exports[`babel-minify Node API override default minify options 1`] = `"function

exports[`babel-minify Node API override nested minify options 1`] = `"function foo(){const a=x(1),b=y(2);return z(a,b)}"`;

exports[`babel-minify Node API preserve default comments 1`] = `"/* @license MIT */(function(){/*! mylib.js */(function(){})()})();"`;

exports[`babel-minify Node API remove comments 1`] = `"var a=10;!function(){}();"`;

exports[`babel-minify Node API simple usage 1`] = `"function foo(){const a=x(1),b=y(2);return z(a,b)}"`;
24 changes: 24 additions & 0 deletions packages/babel-minify/__tests__/node-api-tests.js
Expand Up @@ -28,4 +28,28 @@ describe("babel-minify Node API", () => {
const minifyOpts = { mangle: { keepFnName: false } };
expect(minify(sampleInput, minifyOpts).code).toMatchSnapshot();
});

it("preserve default comments", () => {
const code = `
/* @license MIT */
(function() {
/*! mylib.js */
function a() {}
a();
})();
`;

expect(minify(code, {}).code).toMatchSnapshot();
});

it("remove comments ", () => {
const code = `
/* foo */
var a = 10;

!function(){}() // blah
`;

expect(minify(code, {}).code).toMatchSnapshot();
});
});
30 changes: 25 additions & 5 deletions packages/babel-minify/src/index.js
Expand Up @@ -5,29 +5,49 @@ module.exports = function babelMinify(
input,
// Minify options passed to minifyPreset
// defaults are handled in preset
options = {},
minifyOptions = {},
// overrides and other options
{
minified = true,
inputSourceMap,
sourceMaps = false,
sourceType = "script",
comments = /^\**!|@preserve|@licen(c|s)e|@cc_on/,

// to override the default babelCore used
babel = babelCore,

// to override the default minify preset used
minifyPreset = babelPresetMinify
minifyPreset = babelPresetMinify,

// passthrough to babel
filename,
filenameRelative
} = {}
) {
return babel.transformSync(input, {
babelrc: false,
configFile: false,
presets: [[minifyPreset, options]],
comments: false,
presets: [[minifyPreset, minifyOptions]],
shouldPrintComment(contents) {
return shouldPrintComment(contents, comments);
},
inputSourceMap,
sourceMaps,
minified,
sourceType
sourceType,
filename,
filenameRelative
});
};

function shouldPrintComment(contents, predicate) {
switch (typeof predicate) {
case "function":
return predicate(contents);
case "object":
return predicate.test(contents);
default:
return !!predicate;
}
}
@@ -1,8 +1,6 @@
// FIXME: for some reason, the inner `if` statement gets indented 4 spaces.
`
function foo() {
if (a) {
if (b()) return false;
} else if (c()) return true;
}
`
function foo() {
if (a) {
if (b()) return false;
} else if (c()) return true;
}
@@ -1,7 +1,6 @@
`
function foo() {
if (a) {
if (b()) return false;
} else if (c()) return true;
}
`;
// FIXME: for some reason, the inner `if` statement gets indented 4 spaces.
function foo() {
if (a) {
if (b()) return false;
} else if (c()) return true;
}
22 changes: 0 additions & 22 deletions packages/babel-preset-minify/__tests__/preset-tests.js
Expand Up @@ -33,28 +33,6 @@ describe("preset", () => {
`
);

thePlugin(
"should fix remove comments",
`
var asdf = 1; // test
`,
`
var asdf = 1;
`
);

thePlugin(
"should keep license/preserve annotated comments",
`
/* @license */
var asdf = 1;
`,
`
/* @license */
var asdf = 1;
`
);

thePlugin(
"should fix issue#385 - impure if statements with Sequence and DCE",
`
Expand Down
1 change: 0 additions & 1 deletion packages/babel-preset-minify/src/index.js
Expand Up @@ -99,7 +99,6 @@ function preset(context, _opts = {}) {

return {
minified: true,
comments: false,
presets: [{ plugins }],
passPerPreset: true
};
Expand Down
2 changes: 1 addition & 1 deletion packages/gulp-babel-minify/src/index.js
Expand Up @@ -13,7 +13,7 @@ function gulpBabelMinify(
{
babel = babelCore,
minifyPreset = babelPresetMinify,
comments = /preserve|licen(s|c)e/,
comments = /^\**!|@preserve|@licen(c|s)e|@cc_on/,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Th (s|c) can be shortened to [sc].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will update 👍

sourceType = "script"
} = {}
) {
Expand Down