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

Idea: add a way of warning users about using deprecated functions #541

Open
nfischer opened this issue Nov 1, 2016 · 8 comments
Open

Idea: add a way of warning users about using deprecated functions #541

nfischer opened this issue Nov 1, 2016 · 8 comments
Assignees
Milestone

Comments

@nfischer
Copy link
Member

nfischer commented Nov 1, 2016

Once #524 is merged, it would be good to deprecate shell.exec() due to its vulnerabilities and bugs. While we can mark it as deprecated in the docs, since it's a security vulnerability, we may also want to provide deprecation warnings at runtime.

However, since exec() still has a valid use case, I don't think we should ever get rid of it (so it's soft deprecation). Also, if we're still allowing people to use it, we should provide an API to skip deprecation warnings if they know what they're doing.

One approach we could take:

  • add shell.config.showDeprecationWarnings (defaults to true)
  • then add a function in common) to log a deprecation warning to stderr, only when the above option is true
  • in the deprecated functions, we can call this log function explicitly, or perhaps add a plugin option for deprecated functions of the form:
deprecationMessage: 'foo is deprecated, use bar instead',

This idea is still open for discussion, so don't implement it yet until the "help wanted" label is applied.

@nfischer
Copy link
Member Author

I think the best approach is to implement this as an attribute on plugins/commands. This will also expose one way for plugin authors to deprecate their plugins.

The deprecation message should be shown for these scenarios (in the examples, exec() is the deprecated function):

  • using the shelljs command (shell.exec(...))
  • using the shelljs command in a pipe (shell.cat('file.txt').exec(...))
  • using the shelljs command in global mode (this should be trivial if the first case is addressed)

The best way to do it is probably to check inside common.wrap() for the deprecation flag, and output a warning at runtime.

@freitagbr what are your thoughts on turning deprecation warnings on by default? Or should we wait for the v0.9 release to have warnings on by default?

@freitagbr
Copy link
Contributor

I think we can turn them on by default for the next release. Deprecation warnings for node v7 itself are printed by default, anyway.

@nfischer nfischer added the breaking Breaking change label Feb 3, 2017
@mikeromano38
Copy link

Perhaps it would be nice to also include JSDoc deprecation tags, so that various JS-focused IDEs can hint to the developer of the deprecations. http://usejsdoc.org/tags-deprecated.html

@freitagbr
Copy link
Contributor

Speaking of JSDoc, it has become a sort of standard, so it would be useful to incorporate JSDoc comments in the code base. ShellJS uses a home-grown comment system that converts //@ comments into markdown, specifically for the README.md. However, JSDoc does not seem to have good support for generating markdown, and other modules like jsdoc-to-markdown and jsdox are hit-and-miss in the markdown that is generated.

@nfischer
Copy link
Member Author

No deprecated commands yet, so I'm bumping this back a milestone.

@nfischer nfischer modified the milestones: v0.8.0, v0.9.0 Oct 20, 2017
@nfischer
Copy link
Member Author

Let's put some more thought into solving this. I'd like to implement this with util.deprecate() if possible.

We should use care that --trace-deprecation and --throw-deprecation give meaningful stack traces, which point to the embedding module's code, not internal ShellJS code.

@nfischer
Copy link
Member Author

@freitagbr can you take this? It'd be good to have this finished shortly after #866.


Removing the "breaking" label because this issue is about support for deprecating plugins, this does not track deprecating any particular plugin.

@freitagbr
Copy link
Contributor

Sure, I will tackle this.

nfischer added a commit that referenced this issue Nov 13, 2018
This adds a new wrap() option named `.deprecate`. This currently accepts
a string value, which will be the deprecation message for the plugin.
Deprecation works on standard commands as well as pipes. The call stack
points directly to the deprecated method, and not to any ShellJS
internals.

Fixes #541
Test: This adds unit tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants