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

Implement deprecate API and deprecate APIs #1625

wants to merge 5 commits into from

Conversation

shama
Copy link
Member

@shama shama commented Feb 12, 2018

Ref: gruntjs/rfcs#1

This implements an internal deprecations API that displays a warning in the console if one of the deprecated APIs are used.

To hide deprecation warnings, use --hide-deprecations on the CLI or grunt.option('hide-deprecations', true) in your Gruntfile.

When --stack or grunt.option('stack', true) is enabled, the deprecation warnings will print stack traces out to locate the deprecated APIs.


The following APIs will be publicly deprecated:

  • All functions on grunt.util._
  • All functions on grunt.util.async
  • All functions on grunt.util.namespace
  • All functions on grunt.util.hooker
  • grunt.util.exit
  • grunt.util.toArray
  • grunt.util.repeat
  • All functions on grunt.file.glob
  • All functions on grunt.file.minimatch
  • All functions on grunt.file.findup
  • grunt.file.readYAML
  • grunt.file.readJSON
  • All functions on grunt.event

The following APIs should be considered for deprecation although there isn't simple alternatives to point people to at this time (help?):

  • grunt.util.spawn
  • grunt.util.callbackify
  • grunt.util.kindOf
  • grunt.util.pluralize
  • grunt.util.recurse

Internally Grunt still uses these deprecated APIs. We should address those deprecation warning internally first too.

All deprecations have now been fixed in this PR.


This can be released on next minor release as it is backwards compatible but only adds a new behavior. Removing the deprecated APIs will happen on the next major release.

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.

}
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.

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? 😁

}
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.

@cowboy
Copy link
Member

cowboy commented Feb 12, 2018

Overall, makes sense to me, Thanks, @shama for doing this. I do like the idea, though, of first implementing the warnings in verbose mode only, then in a subsequent minor release, in non-verbose mode, and then finally removing the exposed methods in a future major release. However, I'm not sure how much I care :)

Deprecations live separately from deprecate API
Dont change original val on set
deprecate doesnt accept array, just iterate and call deprecate
obj: grunt.util,
property: 'repeat',
message: 'grunt.util.repeat has been deprecated. Please use ' +
'`new Array(num + 1).join(str || \' \')` or another library.'
Copy link
Member

Choose a reason for hiding this comment

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

lol, didn't know this existed

Copy link
Member

Choose a reason for hiding this comment

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

grunt.util.repeat that is

@vladikoff
Copy link
Member

Code looks good, but I will need to run this locally to make sure it deprecates. I Will try to get to that soon

Base automatically changed from master to main March 22, 2021 14:45
"glob": "~7.0.0",
"grunt-cli": "~1.2.0",
"grunt-known-options": "~1.1.0",
"grunt-legacy-log": "~1.0.0",
"grunt-legacy-util": "~1.0.0",
"iconv-lite": "~0.4.13",
"js-yaml": "~3.5.2",
"lodash": "~4.17.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need lodash?

Copy link
Member Author

Choose a reason for hiding this comment

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

5 years ago we probably did.

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

Successfully merging this pull request may close these issues.

4 participants