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

feat: add the eslint-scope package #615

Merged
merged 8 commits into from
Aug 15, 2024
Merged

feat: add the eslint-scope package #615

merged 8 commits into from
Aug 15, 2024

Conversation

snitin315
Copy link
Contributor

Refers #609

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

The moved files look good. We also need to update release-please-config.json and .release-please-manifest.json to ensure that release-please knows about this package.

languageOptions: {
globals: {
...globals.mocha
}
},
plugins: {
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to add more to this config?

Copy link
Contributor Author

@snitin315 snitin315 Jul 23, 2024

Choose a reason for hiding this comment

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

Yeah, this is to match the eslint-scope ESLint config.

I merged both the configs, but, I think we can have different config for packages/espree/tests/lib/** & packages/eslint-scope/tests/lib/** too.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can have different config for packages/espree/tests/lib/** & packages/eslint-scope/tests/lib/**

Sounds good to me to have different configs, because eslint-scope tests use chai while espree tests don't (I'm not sure why chai was listed in espree dev deps).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, also removed chai from espree's dev dep.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Looks like we also need to add eslint-scope to the issue templates.

@snitin315
Copy link
Contributor Author

Updated 👍🏻

@mdjermanovic
Copy link
Member

Looks like npm-license expects dependencies to be installed in node_modules of the start path. But in a workspaces monorepo, dependencies are installed in top-level node_modules.

When I add console.log(Object.keys(deps)); here, it logs only:

In the old eslint-scope repo, it logs:

So it seems that the license check effectively wouldn't work? I'm not sure how this could be fixed and whether we need a license check for the eslint-scope package. We have a license check in eslint/eslint, and eslint-scope seems to be the only other package with this check.

@nzakas
Copy link
Member

nzakas commented Jul 29, 2024

In a monorepo, all dependencies end up in the top-level node_modules, so if we do the license check from the root of the repo, I think it should work.

@snitin315
Copy link
Contributor Author

Providing the top-level path here also didn't work, it was then outputting [ '[email protected]' ] only.

For now, I have passed additional metadata (See 6405974) to keep the behavior the same as eslint-scope repo, I think we can follow up on this and see how it can be improved for monorepo.

@aladdin-add aladdin-add self-requested a review August 6, 2024 09:33
aladdin-add
aladdin-add previously approved these changes Aug 7, 2024
packages/eslint-scope/Makefile.js Outdated Show resolved Hide resolved
@@ -0,0 +1,110 @@
[![npm version](https://img.shields.io/npm/v/eslint-scope.svg)](https://www.npmjs.com/package/eslint-scope)
[![Downloads](https://img.shields.io/npm/dm/eslint-scope.svg)](https://www.npmjs.com/package/eslint-scope)
[![Build Status](https://github.com/eslint/eslint-scope/workflows/CI/badge.svg)](https://github.com/eslint/eslint-scope/actions)
Copy link
Member

Choose a reason for hiding this comment

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

need to update the link after the repo being renamed.

Suggested change
[![Build Status](https://github.com/eslint/eslint-scope/workflows/CI/badge.svg)](https://github.com/eslint/eslint-scope/actions)
[![Build Status](https://github.com/eslint/eslint-scope/workflows/CI/badge.svg)](https://github.com/eslint/js/actions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's update it only after the repo is renamed,

{
"name": "eslint-scope",
"description": "ECMAScript scope analyzer for ESLint",
"homepage": "http://github.com/eslint/eslint-scope",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"homepage": "http://github.com/eslint/eslint-scope",
"homepage": "https://github.com/eslint/js#readme",

"engines": {
"node": "^18.18.0 || ^20.9.0 || >=21.1.0"
},
"repository": "eslint/eslint-scope",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"repository": "eslint/eslint-scope",
"repository": "eslint/js",

"repository": "eslint/eslint-scope",
"funding": "https://opencollective.com/eslint",
"bugs": {
"url": "https://github.com/eslint/eslint-scope/issues"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"url": "https://github.com/eslint/eslint-scope/issues"
"url": "https://github.com/eslint/js/issues"

@nzakas
Copy link
Member

nzakas commented Aug 13, 2024

@snitin315 are you still working on this?

@snitin315
Copy link
Contributor Author

@nzakas Yes, somehow I missed the last review notification. I've updated the PR, PTAL.

@aladdin-add Let's change the eslint/eslint-scope to eslint/js in a follow-up PR for all packages, once the repository has been renamed.

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, pending @nzakas re-review. thanks!

@nzakas
Copy link
Member

nzakas commented Aug 15, 2024

All right, moving this forward. I will rename the repo now.

@nzakas nzakas merged commit 2ecfb8b into main Aug 15, 2024
11 checks passed
@nzakas nzakas deleted the feat/eslint-scope-package branch August 15, 2024 15:05
@github-actions github-actions bot mentioned this pull request Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

4 participants