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

Implement deprecate API and deprecate APIs #1625

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/grunt.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ var verbose = grunt.verbose = log.verbose;
grunt.package = require('../package.json');
grunt.version = grunt.package.version;

// Include internal deprecate lib that adds deprecations
require('./grunt/deprecate');

// Expose specific grunt lib methods on grunt.
function gExpose(obj, methodName, newMethodName) {
grunt[newMethodName || methodName] = obj[methodName].bind(obj);
Expand Down
116 changes: 116 additions & 0 deletions lib/grunt/deprecate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
var grunt = require('../grunt');

function deprecate(obj, property, message) {
if (Array.isArray(obj)) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this "if array" logic, I'd just call .forEach on the array of to-be-deprecated items on line 38.

obj.forEach(function(item) {
deprecate(item.obj, item.property, item.message);
});
return;
}
var logged = false;
function warn() {
var hideDeprecation = grunt.option('hide-deprecations');
Copy link
Member

Choose a reason for hiding this comment

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

Can you look at grunt.option('hide-deprecations') at the top of deprecate and just return there if it's true?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put it in the warn to try to catch later calls to grunt.option('hide-deprecations', true). So you could have the option to potentially hide warnings from other modules but not your own (prevent your team from using deprecated APIs but can't fix deprecations in other APIs).

But I might be complicating it unnecessarily, let me know and I'll move it out.

if (!hideDeprecation && !logged) {
if (grunt.option('stack')) {
grunt.log.warn(Error(message).stack);
} else {
grunt.log.warn(message);
}
logged = true;
}
}
var orig = obj[property];
Object.defineProperty(obj, property, {
enumerable: true,
configurable: true,
set: function(val) {
warn();
orig = val;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe don't call the value orig if it can be changed? 😁

},
get: function() {
warn();
return orig;
}
});
}
module.exports = deprecate;

deprecate([
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to call deprecate in a different file than where it's defined... but perhaps I'm overthinking things!

Copy link
Member Author

Choose a reason for hiding this comment

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

Separating the API from the apply makes sense to me. I moved the deprecations into their own file called deprecations.js.

{
obj: grunt.util,
property: '_',
message: 'grunt.util._ has been deprecated. Please install and require ' +
'"lodash" directly. https://www.npmjs.com/package/lodash'
},
{
obj: grunt.util,
property: 'async',
message: 'grunt.util.async has been deprecated. Please install and require ' +
'"async" directly. https://www.npmjs.com/package/async'
},
{
obj: grunt.util,
property: 'namespace',
message: 'grunt.util.namespace has been deprecated. Please install and ' +
'require "getobject" directly. https://www.npmjs.com/package/getobject'
},
{
obj: grunt.util,
property: 'hooker',
message: 'grunt.util.hooker has been deprecated. Please install and require ' +
'"hooker" directly. https://www.npmjs.com/package/hooker'
},
{
obj: grunt.util,
property: 'exit',
message: 'grunt.util.exit has been deprecated. Please install and require ' +
'"exit" directly. https://www.npmjs.com/package/exit'
},
{
obj: grunt.util,
property: 'toArray',
message: 'grunt.util.toArray has been deprecated. Please install and ' +
'require "lodash.toarray" directly. https://www.npmjs.com/package/lodash.toarray'
},
{
obj: grunt.util,
property: 'repeat',
message: 'grunt.util.repeat has been deprecated. Please use ' +
'`new Array(num + 1).join(str || \' \')` or another library.'
},
{
obj: grunt.file,
property: 'glob',
message: 'grunt.file.glob has been deprecated. Please install and require ' +
'"glob" directly. https://www.npmjs.com/package/glob'
},
{
obj: grunt.file,
property: 'minimatch',
message: 'grunt.file.minimatch has been deprecated. Please install and ' +
'require "minimatch" directly. https://www.npmjs.com/package/minimatch'
},
{
obj: grunt.file,
property: 'findup',
message: 'grunt.file.findup has been deprecated. Please install and require ' +
'"findup-sync" directly. https://www.npmjs.com/package/findup-sync'
},
{
obj: grunt.file,
property: 'readYAML',
message: 'grunt.file.readYAML has been deprecated. Please install and ' +
'require "js-yaml" directly. https://www.npmjs.com/package/js-yaml'
},
{
obj: grunt.file,
property: 'readJSON',
message: 'grunt.file.readJSON has been deprecated. Please use require("file.json") directly.'
},
{
obj: grunt,
property: 'event',
message: 'grunt.event has been deprecated. Please install and require ' +
'"eventemitter2" directly. https://www.npmjs.com/package/eventemitter2'
},
]);
66 changes: 66 additions & 0 deletions test/grunt/deprecate_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
'use strict';

var grunt = require('../../lib/grunt');
var deprecate = require('../../lib/grunt/deprecate');

exports.deprecate = {
setUp: function(done) {
this.oldGruntLogWarn = grunt.log.warn;
this.oldHideDeprecations = grunt.option('hide-deprecations');
this.oldStack = grunt.option('stack');
grunt.option('hide-deprecations', false);
grunt.option('stack', false);
var api = {val: function() {}};
var util = {api: api};
this.fixture = {util: util};
done();
},
tearDown: function(done) {
this.fixture = null;
grunt.log.warn = this.oldGruntLogWarn;
grunt.option('hide-deprecations', this.oldHideDeprecations);
grunt.option('stack', this.oldStack);
done();
},
'deprecate warning on get': function(test) {
test.expect(1);
grunt.log.warn = function(message) {
test.equal(message, 'this api is deprecated', 'grunt.log.warn should have got the deprecation message when getting a deprecated api.');
test.done();
};
deprecate(this.fixture.util, 'api', 'this api is deprecated');
this.fixture.util.api.val();
},
'deprecate warning on set': function(test) {
test.expect(1);
grunt.log.warn = function(message) {
test.equal(message, 'this api is deprecated', 'grunt.log.warn should have got the deprecation message when setting a deprecated api.');
test.done();
};
deprecate(this.fixture.util, 'api', 'this api is deprecated');
this.fixture.util.api = {};
},
'hide deprecations': function(test) {
test.expect(1);
grunt.option('hide-deprecations', true);
grunt.log.warn = function(message) {
test.equal(message, 'did not show deprecation message', 'grunt.log.warn should not have displayed a deprecation message with hide-deprecations enabled.');
test.done();
};
deprecate(this.fixture.util, 'api', 'this api is deprecated');
this.fixture.util.api.val();
grunt.log.warn('did not show deprecation message');
},
'deprecations with stack trace': function(test) {
test.expect(2);
grunt.option('stack', true);
grunt.log.warn = function(message) {
message = message.split('\n');
test.equal(message[0], 'Error: this api is deprecated', 'grunt.log.warn should not have displayed a deprecation message with stack trace.');
test.ok(message.length > 1);
test.done();
};
deprecate(this.fixture.util, 'api', 'this api is deprecated');
this.fixture.util.api.val();
},
};