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

Breaking: Make require('eslint').linter non-enumerable (fixes #9270) #9692

Merged
merged 3 commits into from Mar 22, 2018

Conversation

Projects
7 participants
@j-f1
Contributor

j-f1 commented Dec 6, 2017

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

#9270

What changes did you make? (Give an overview)
I made require('eslint').linter non-enumerable so people aren’t tempted to use it.

Is there anything you'd like reviewers to focus on?
Is this actually a breaking change? The ESLint API is small enough that I doubt anyone uses Object.keys on it, and it would be unfortunate to make this wait until v5, which would probably push the deprecation date back another major version.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Dec 8, 2017

We prefer to err on the side of caution here, so I would consider this a breaking change.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Dec 8, 2017

Also, please fix the lint errors when you get a chance (but no rush for sure, since we aren't about to do a major release). 😄

Thanks for contributing!

@j-f1 j-f1 changed the title from Breaking?: Make require('eslint').linter non-enumerable (fixes #9270) to Breaking: Make require('eslint').linter non-enumerable (fixes #9270) Dec 8, 2017

@platinumazure

Could the out.txt file be removed? Thanks!

@j-f1

This comment has been minimized.

Contributor

j-f1 commented Dec 11, 2017

@platinumazure All gone!

Would you like me to write a few tests for this?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Dec 11, 2017

@j-f1

Would you like me to write a few tests for this?

Excellent question, I'm not sure if that would be valuable or not. @eslint/eslint-team, any thoughts on this?

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Dec 11, 2017

I suppose it would be nice to add a test.

lib/api.js Outdated
Object.defineProperty(module.exports, "linter", {
enumerable: false,
configurable: false, // Don't let people bypass the warning

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Dec 11, 2017

Member

There is no longer a warning, so I think this comment is a bit misleading now.

@gyandeeps gyandeeps added this to the v5.0.0 milestone Dec 14, 2017

@j-f1

This comment has been minimized.

Contributor

j-f1 commented Jan 5, 2018

@platinumazure does your review still stand, or is this good to go once 5.0 is ready?

@platinumazure

LGTM for merging in a major release. Thanks!

lib/api.js Outdated
Object.defineProperty(module.exports, "linter", {
enumerable: false,
configurable: false,

This comment has been minimized.

@michaelficarra

michaelficarra Jan 5, 2018

Member

Why non-configurable?

This comment has been minimized.

@j-f1

j-f1 Jan 6, 2018

Contributor

So people won’t override it.

I originally had it print a warning when first accessed, but I was asked to remove that feature.

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Jan 6, 2018

Member

Could you elaborate on what this is intended to protect against? I don't see how it would be advantageous for a user to tamper with linter in particular -- if someone really wanted to monkeypatch for some reason, they could mutate the eslint.linter object or Linter.prototype anyway.

I think it would be reasonable for us to try to guard against monkeypatching in general (e.g. by freezing exported objects), but it's not clear to me that only doing this for linter has any benefit.

This comment has been minimized.

@j-f1

j-f1 Jan 6, 2018

Contributor

Fixed.

@Arcanemagus

This comment has been minimized.

Arcanemagus commented Jan 8, 2018

We prefer to err on the side of caution here, so I would consider this a breaking change.

We were actually using this non-public property in the linter-eslint Atom plugin... but CLIEngine#getRules removed the need for that (as soon as AtomLinter/linter-eslint#1067 gets merged at least 😆).

@j-f1

This comment has been minimized.

Contributor

j-f1 commented Jan 8, 2018

@Arcanemagus Don’t worry — it’s still there, it just won’t show up in Object.keys.

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

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 22, 2018

Closing and reopening to retrigger the release-monitor status check.

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

@not-an-aardvark not-an-aardvark moved this from Done to Ready to merge in v5.0.0 Mar 22, 2018

@not-an-aardvark not-an-aardvark merged commit c383bc5 into eslint:master Mar 22, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

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

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 22, 2018

Thanks for contributing!

@j-f1 j-f1 deleted the j-f1:hide-eslint.linter branch 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.