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

Chore (upgrade eslint config and packages) #3409

Merged

Conversation

jmetev1
Copy link
Contributor

@jmetev1 jmetev1 commented Jun 7, 2023

fixes #2583
I updated linting packages. And of course that comes with some changes. I broke up the autofixes vs manual fixes by commit. Autofix did some of the vars, but not all of them. What do you want to do with 144 remaining?

Also i see you're hiring. Can i get a referral?

Checklist

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 7, 2023

💚 CLA has been signed

@github-actions github-actions bot added agent-nodejs Make available for APM Agents project planning. community triage labels Jun 7, 2023
"eslint-plugin-license-header": "^0.6.0",
"eslint-plugin-node": "^11.1.0",
"eslint-plugin-promise": "^4.3.1",
"eslint-plugin-standard": "^4.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no longer needed with this eslint-config-standard

@jmetev1
Copy link
Contributor Author

jmetev1 commented Jun 7, 2023

i have now signed cla

@jmetev1 jmetev1 marked this pull request as ready for review June 7, 2023 23:56
@trentm trentm self-assigned this Jun 8, 2023
@trentm trentm removed the triage label Jun 8, 2023
@trentm
Copy link
Member

trentm commented Jun 8, 2023

Hi @jmetev1. Thanks very much for the PR. There are a lot of changes here, including lint/style changes, so I have a fair amount of feedback.
Thank you for splitting manual and auto changes between commits.

no-var

(standard/standard#633)

We discussed internally for a bit, and we'd like to keep this off. I.e. we'd like to not take the large set of changes to switch to const/let. The exception is with the "examples/" dir, where we'll apply the rule and change current var usages over. I have a patch below that explains our reasoning.

array-callback-return

(standard/standard#859)

I'd like to keep this set to "error", and then revisit the few places we hit this:

./lib/instrumentation/modules/graphql.js
  175:33  error  Expected to return a value at the end of function  array-callback-return
  176:83  error  Function expected a return value                   array-callback-return
  177:42  error  Function expected a return value                   array-callback-return

./lib/redact-secrets/index.js
  41:30  error  Expected to return a value in function  array-callback-return

./test/_utils.js
  38:12  error  Expected to return a value at the end of function  array-callback-return

Coordinating

  1. Here is a suggested starter patch for setting "no-var" and a couple other small changes: https://gist.github.com/trentm/d610966e3cdd02ec8ff810f1b30da511
    If you would prefer, I can make commits to your feature branch. Please let me know.

  2. Then your commit 1727021 with auto lint fixes should be reverted.

  3. Then there will be some autofixes for "no-var" just in the "examples/" subdir to apply.

  4. Then we can work through the array-callback-return issues. I am happy to work through those. The one in redact-secrets is the only one I have to poke around to relearn.

@jmetev1
Copy link
Contributor Author

jmetev1 commented Jun 8, 2023

I'm afraid with this patch you've gone out of my git depth. I tried to google but am unsure if this is something I'm supposed to be able to apply automatically. But feel free to update my branch.

@trentm
Copy link
Member

trentm commented Jun 9, 2023

But feel free to update my branch.

Will do. Thanks.

@jmetev1
Copy link
Contributor Author

jmetev1 commented Jun 9, 2023

but i am curious, what was i supposed to do with your gist? i was looking around here, but had no luck https://gist.github.com/nepsilon/22bc62a23f785716705c

@trentm
Copy link
Member

trentm commented Jun 9, 2023

but i am curious, what was i supposed to do with your gist? i was looking around here, but had no luck https://gist.github.com/nepsilon/22bc62a23f785716705c

That gist has it right. You would have needed to download my gist to a file (using the "Raw" link): https://gist.githubusercontent.com/trentm/d610966e3cdd02ec8ff810f1b30da511/raw/10433a76f380c9724b932fe0ef9205dc97940375/the.patch
and then applied that to your git clone with git apply path/to/downloaded/the.patch.

@jmetev1
Copy link
Contributor Author

jmetev1 commented Jun 9, 2023

ohhhh i forget about raw option. i tried that but without raw part.

@trentm trentm requested a review from david-luna June 9, 2023 22:01
@trentm
Copy link
Member

trentm commented Jun 9, 2023

@elasticmachine, run elasticsearch-ci/docs

@trentm
Copy link
Member

trentm commented Jun 13, 2023

@elasticmachine, run elasticsearch-ci/docs

@trentm trentm merged commit 65c4b82 into elastic:main Jun 14, 2023
@trentm
Copy link
Member

trentm commented Jun 14, 2023

Thanks very much @jmetev1

@jmetev1
Copy link
Contributor Author

jmetev1 commented Jun 15, 2023

no problem

trentm added a commit that referenced this pull request Jun 27, 2023
The upgrade to eslint@8 in #3409 subtly broken peer-dependencies.
`npm ls` unhelpfully does not complain, but `npm ls -a` shows some
errors, and `./dev-utils/make-distribution.sh` was broken.

The upgrade bumped "eslint-config-standard" from ^14.1.1 to ^16.
However, v16 has:
    "peerDependencies": {
      "eslint": "^7.12.1",
      "eslint-plugin-import": "^2.22.1",
      "eslint-plugin-node": "^11.1.0",
      "eslint-plugin-promise": "^4.2.1 || ^5.0.0"
    },
so is incompatible with eslint@8. We need eslint-config-standard@17,
which has:
    "peerDependencies": {
      "eslint": "^8.0.1",
      "eslint-plugin-import": "^2.25.2",
      "eslint-plugin-n": "^15.0.0 || ^16.0.0 ",
      "eslint-plugin-promise": "^6.0.0"
    },

Note that also requires a change from eslint-plugin-node (unmaintained)
to the replacement "eslint-plugin-n".
trentm added a commit that referenced this pull request Jul 4, 2023
The upgrade to eslint@8 in #3409 subtly broke peer-dependencies.
`npm ls` unhelpfully does not complain, but `npm ls -a` shows some
errors, and `./dev-utils/make-distribution.sh` was broken.

The upgrade bumped "eslint-config-standard" from ^14.1.1 to ^16.
However, v16 has:
    "peerDependencies": {
      "eslint": "^7.12.1",
      "eslint-plugin-import": "^2.22.1",
      "eslint-plugin-node": "^11.1.0",
      "eslint-plugin-promise": "^4.2.1 || ^5.0.0"
    },
so is incompatible with eslint@8. We need eslint-config-standard@17,
which has:
    "peerDependencies": {
      "eslint": "^8.0.1",
      "eslint-plugin-import": "^2.25.2",
      "eslint-plugin-n": "^15.0.0 || ^16.0.0 ",
      "eslint-plugin-promise": "^6.0.0"
    },

Also:
- eslint-plugin-node (unmaintained) is replaced by "eslint-plugin-n"
- eslint-config-standard@17 added https://eslint.org/docs/latest/rules/object-shorthand
  as a warning. It'll become "error" in a subsequent major.
- eslint@8 means support for top-level await, so there are some 
  files we no longer need to skip
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
We have turned *off* the "no-var" rule that was added to
the "standard" config with this upgrade. See .eslintrc.json
for justification. We are applying "no-var" in examples/...

Using eslint@8 bumps the min node for linting to 12.20.0.

Co-authored-by: Trent Mick <trent.mick@elastic.co>
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
…ic#3449)

The upgrade to eslint@8 in elastic#3409 subtly broke peer-dependencies.
`npm ls` unhelpfully does not complain, but `npm ls -a` shows some
errors, and `./dev-utils/make-distribution.sh` was broken.

The upgrade bumped "eslint-config-standard" from ^14.1.1 to ^16.
However, v16 has:
    "peerDependencies": {
      "eslint": "^7.12.1",
      "eslint-plugin-import": "^2.22.1",
      "eslint-plugin-node": "^11.1.0",
      "eslint-plugin-promise": "^4.2.1 || ^5.0.0"
    },
so is incompatible with eslint@8. We need eslint-config-standard@17,
which has:
    "peerDependencies": {
      "eslint": "^8.0.1",
      "eslint-plugin-import": "^2.25.2",
      "eslint-plugin-n": "^15.0.0 || ^16.0.0 ",
      "eslint-plugin-promise": "^6.0.0"
    },

Also:
- eslint-plugin-node (unmaintained) is replaced by "eslint-plugin-n"
- eslint-config-standard@17 added https://eslint.org/docs/latest/rules/object-shorthand
  as a warning. It'll become "error" in a subsequent major.
- eslint@8 means support for top-level await, so there are some 
  files we no longer need to skip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning. community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade to eslint@8
2 participants