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

Jump to bottom

Chore: update dev deps to latest #14624

Merged
merged 4 commits into from Aug 5, 2021
Merged

Chore: update dev deps to latest #14624

merged 4 commits into from Aug 5, 2021

Conversation

aladdin-add
Copy link
Member

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

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?

nzakas
nzakas reviewed Aug 5, 2021
View reviewed changes
.markdownlint.yml Outdated
@@ -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

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

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

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

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

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 •
edited

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

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 •
edited

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

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

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

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 August 5, 2021 11:34
@bmish
Copy link
Sponsor Member

bmish commented Aug 5, 2021 •
edited

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

  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
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 August 5, 2021 18:55
@bmish bmish changed the title Chore: update devdeps to latest Chore: update dev deps to latest Aug 5, 2021
@nzakas nzakas merged commit f7b4a3f into eslint:master Aug 5, 2021
@snitin315
Copy link
Contributor

snitin315 commented Aug 6, 2021 •
edited

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

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

@aladdin-add
Copy link
Member Author

have you reinstalled the deps?

@snitin315
Copy link
Contributor

@aladdin-add yes

@aladdin-add
Copy link
Member Author

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

@snitin315
Copy link
Contributor

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.
Reviewers

@bmish bmish bmish approved these changes

@nzakas nzakas nzakas approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aladdin-add @nzakas @bmish @snitin315