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

Make `eslint.linter` non-enumerable #9270

Closed
not-an-aardvark opened this Issue Sep 9, 2017 · 4 comments

Comments

Projects
3 participants
@not-an-aardvark
Member

not-an-aardvark commented Sep 9, 2017

When using ESLint on master and Node 8.4.0, running require("eslint") in the REPL produces this output:

$ node
> require('eslint')
{ linter:
   Linter {
     sourceCode: null,
     version: '4.6.1',
     rules: Rules { _rules: [Object] },
     environments: Environments { _environments: [Object] } },
  Linter: [Function: Linter],
  CLIEngine: { [Function: CLIEngine] version: '4.6.1', getFormatter: [Function: getFormatter] },
  RuleTester: { [Function: RuleTester] [Symbol(it)]: null, [Symbol(describe)]: null },
  SourceCode: [Function: SourceCode] }

Note that eslint.linter is the first property that appears in the list, and eslint.Linter comes afterwards. This could be confusing for users who are figuring out how to use the API with the REPL, because they might be inclined to use eslint.linter despite the fact that eslint.linter is deprecated. The distinction between eslint.linter and eslint.Linter is also likely to be unclear.

To avoid this, I think we should make eslint.linter non-enumerable. That way, it will still be available to old integrations using eslint.linter, but it's unlikely to create confusion for new users.

I'm unsure whether this should be considered a breaking change -- it could change the behavior of something like Object.keys(require("eslint")), but I don't know why anyone would do that.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Oct 24, 2017

TSC Summary: This issue proposes making the deprecated eslint.linter property non-enumerable, to make it clearer to users that they should use eslint.Linter instead. This could be a breaking change for some unusual uses of the API, such as Object.keys(require('eslint')).

TSC Question: Should we accept this issue?

@kaicataldo

This comment has been minimized.

Member

kaicataldo commented Oct 26, 2017

TSC Resolution: This is now accepted as a breaking change.

@not-an-aardvark not-an-aardvark added this to Accepted in v5.0.0 Oct 26, 2017

@j-f1

This comment has been minimized.

Contributor

j-f1 commented Dec 2, 2017

I’m looking into doing this.

EDIT: See j-f1@5f8ff0b for my initial implementation.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Dec 2, 2017

@j-f1 Thanks! To be honest, I'm not sure we should add a deprecation warning -- the main goal of this change is to deter new users, not to issue warnings for old users. I haven't looked into how many integrations are still using eslint.linter, but in general deprecation warnings can occasionally cause problems (e.g. if all messages from stdout are relayed to the user in an obtrusive way).

If we do add a deprecation warning, I think we should use something like process.emitWarning so that we respect Node's --no-warnings CLI flag.

j-f1 added a commit to j-f1/forked-eslint that referenced this issue Dec 6, 2017

j-f1 added a commit to j-f1/forked-eslint that referenced this issue Dec 8, 2017

j-f1 added a commit to j-f1/forked-eslint that referenced this issue Dec 11, 2017

@not-an-aardvark not-an-aardvark moved this from Accepted to Ready to Merge in v5.0.0 Feb 2, 2018

v5.0.0 automation moved this from Ready to merge to Done Mar 22, 2018

@eslint eslint bot locked and limited conversation to collaborators Sep 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.