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: update dev deps to latest #14624

Merged
merged 4 commits into from Aug 5, 2021

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented May 25, 2021

Prerequisites checklist

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

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

What changes did you make? (Give an overview)

upgrade all dev deps to latest.

Is there anything you'd like reviewers to focus on?

no.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label May 25, 2021
@aladdin-add aladdin-add added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion upgrade This change is related to a dependency upgrade chore This change is not user-facing and removed triage An ESLint team member will look at this issue soon labels May 25, 2021
@nzakas
Copy link
Member

nzakas commented Aug 5, 2021

Looks like we lost this. Do you want to update?

@@ -16,6 +16,7 @@ MD029: false # Ordered list item prefix
MD030: false # Spaces after list markers
MD033: false # Allow inline HTML
MD034: false # Bare URL used
Copy link
Member

@nzakas nzakas Aug 5, 2021

Choose a reason for hiding this comment

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

This change is unrelated and should be removed from this PR

Copy link
Sponsor Member

@bmish bmish Aug 5, 2021

Choose a reason for hiding this comment

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

I'm guessing it's related to updating markdownlint.

Copy link
Member Author

@aladdin-add aladdin-add Aug 5, 2021

Choose a reason for hiding this comment

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

yes, it's a new added rule that we were not following.

Copy link
Member

@nzakas nzakas Aug 5, 2021

Choose a reason for hiding this comment

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

Right. I’m saying such a change doesn’t belong in a PR that is just to upgrade dependencies.

Copy link
Member Author

@aladdin-add aladdin-add Aug 5, 2021

Choose a reason for hiding this comment

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

to not change the config, pushed a commit to fix the md037 issues: b8b032a.

Copy link
Sponsor Member

@bmish bmish Aug 5, 2021

Choose a reason for hiding this comment

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

I would actually recommend just ignoring CHANGELOG.md which is autogenerated in a .markdownlintignore file like this: https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/.markdownlintignore

Copy link
Member Author

@aladdin-add aladdin-add Aug 5, 2021

Choose a reason for hiding this comment

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

sounds good!

Copy link
Sponsor Member

@bmish bmish Aug 5, 2021

Choose a reason for hiding this comment

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

@aladdin-add it looks like this codebase is using markdownlint via CLI and also via its NodeJS API in Makefile.js. Only the CLI version will automatically consider the .markdownlintignore file. I think we should probably move this dependency upgrade to a separate PR since it is becoming more complicated. I can take that on if that sounds good.

Copy link
Member Author

@aladdin-add aladdin-add Aug 5, 2021

Choose a reason for hiding this comment

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

agreed to put in another PR! I will revert the markdownlint version.

Copy link
Sponsor Member

@bmish bmish Aug 5, 2021

Choose a reason for hiding this comment

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

Separate PR to update markdownlint: #14883

@bmish
Copy link
Sponsor Member

bmish commented Aug 5, 2021

I'm interested to see this get in but looks like it's pretty out-of-date with master.

@aladdin-add
Copy link
Member Author

aladdin-add commented Aug 5, 2021

sure, will update later. :)

@aladdin-add aladdin-add force-pushed the chore/upgrade-dev-deps branch 4 times, most recently from 3e70278 to 335671a Compare Aug 5, 2021
@bmish
Copy link
Sponsor Member

bmish commented Aug 5, 2021

It looks like this is still missing some updates like glob-parent (#14879). Any reason that's not included? Looks like you still don't have the latest package.json from master.

@aladdin-add
Copy link
Member Author

aladdin-add commented Aug 5, 2021

  1. the PR was to upgrade dev deps, but not the deps(e.g. glob-parent).
  2. some dev deps were not upgraded as breaking changes. e.g. eslint-plugin-jsdoc dropped the node v10, but eslint v7 still supports it.

bmish
bmish approved these changes Aug 5, 2021
@bmish
Copy link
Sponsor Member

bmish commented Aug 5, 2021

@aladdin-add got it, thanks!

@aladdin-add aladdin-add force-pushed the chore/upgrade-dev-deps branch 2 times, most recently from f9e3c70 to 335671a Compare Aug 5, 2021
@bmish bmish changed the title Chore: update devdeps to latest Chore: update dev deps to latest Aug 5, 2021
nzakas
nzakas approved these changes Aug 5, 2021
@nzakas nzakas merged commit f7b4a3f into eslint:master Aug 5, 2021
13 checks passed
@snitin315
Copy link
Contributor

snitin315 commented Aug 6, 2021

Now npm run lint is failing on master with this, we may have to revert this PR.

Screenshot 2021-08-06 at 7 04 14 AM

@snitin315
Copy link
Contributor

snitin315 commented Aug 6, 2021

Strange, it's failing locally only. All good in CI

@aladdin-add
Copy link
Member Author

aladdin-add commented Aug 6, 2021

have you reinstalled the deps?

@snitin315
Copy link
Contributor

snitin315 commented Aug 6, 2021

@aladdin-add yes

@aladdin-add
Copy link
Member Author

aladdin-add commented Aug 6, 2021

what's your node.js/npm version? maybe you were running on a unwanted version.

@snitin315
Copy link
Contributor

snitin315 commented Aug 7, 2021

Node v14, npm v6.

rm -rf ./node_modules && npm install worked for me 👍🏻 , all good now.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 2, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion upgrade This change is related to a dependency upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants