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 eslint/core package #68

Merged
merged 11 commits into from
Jun 26, 2024
Merged

feat: Add eslint/core package #68

merged 11 commits into from
Jun 26, 2024

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Jun 24, 2024

Prerequisites checklist

What is the purpose of this pull request?

This package creates the @eslint/core package, which is the future home of the runtime-agnostic rewritten core of ESLint.

Right now it just exports types for language plugins.

What changes did you make? (Give an overview)

  • Created the packaged
  • Updated the release-please script
  • Created type definitions

Related Issues

fixes #62

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

I wasn't sure if we should be testing the type definitions somehow? I'd defer to someone with more TypeScript knowledge than I on how that might be done.

packages/core/jsr.json Show resolved Hide resolved
packages/core/README.md Outdated Show resolved Hide resolved
packages/core/package.json Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
packages/core/src/types.ts Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
packages/core/src/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

[Docs] Following #52, it'd be good to have a decision doc for licensing. Even if it's just "we need / want to use the same as existing ESLint licenses".

Copy link
Member Author

Choose a reason for hiding this comment

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

There was no decision made. The OpenJS Foundation says any new projects must be licensed under Apache 2.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a source that can be linked?

Copy link
Member Author

Choose a reason for hiding this comment

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

Linked from where?

And I have no idea. We've been part of the foundation for a long time and this was communicated early on.

There might be something somewhere in https://github.com/openjs-foundation if you feel like going on a search.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Jun 26, 2024

Choose a reason for hiding this comment

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

Linked from where?

Proposal: a decision doc, for visibility?

nzakas and others added 5 commits June 26, 2024 10:22
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

(posting some quick comment-questions in lieu of a full review)

packages/core/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Following #52 and #73: I don't feel able to review this file without understanding how this is meant to play with @types/eslint over time. Is it intentionally different? Have the tradeoffs been discussed somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

We weren't consulted on the creation of @types/eslint, so I consider those to be non-canonical types. As we move forward, we'll be creating types the way we want and using those. Maybe @types/eslint will get smart and start importing our types to flesh out the package, but either way, I'm not going to worry too much about it.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Jun 26, 2024

Choose a reason for hiding this comment

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

non-canonical types

Practically speaking, those and typescript-eslint are canonical types, as those are what the entire community uses at the moment. If the decision is to explicitly not attempt backwards compat, sure, that's a reasonable decision - but I don't think they can be ignored in decision-making summarily.

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit e3d309d into main Jun 26, 2024
14 checks passed
@mdjermanovic mdjermanovic deleted the core branch June 26, 2024 16:05
@github-actions github-actions bot mentioned this pull request Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Change Request: eslint/core as types-only package (to start)
4 participants