Skip to content

fix!: remove rollup and extraneous types #383

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

xbinaryx
Copy link

Prerequisites checklist

What is the purpose of this pull request?

Simplify build process and type management by removing unnecessary Rollup bundling for this ESM package.

What changes did you make? (Give an overview)

Removed Rollup, using tsc to emit code/types directly to dist

Related Issues

Closes #374

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

This change causes a breaking issue because tsc splits type declarations across multiple files instead of bundling them in index.d.ts like Rollup did. Some types are no longer exported from the main entry point. Should we re-export them in index.js for backwards compatibility, or accept this as a breaking change? Would appreciate team feedback.

@nzakas
Copy link
Member

nzakas commented May 21, 2025

Let's make sure that the types test is passing. That should cover our bases when it comes to exported types.

npm run test:types (note: this is currently failing in CI)

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage May 21, 2025
@lumirlumir
Copy link
Member

lumirlumir commented May 21, 2025

Thanks for sending a PR! I just noted down what I noticed.


Currently, MarkdownNode, SourceLocation, and SourceRange are missing.

image


Also, many of the types that were originally exported from index.d.ts are now missing. For example:

image

We used to be able to access these exported types, but now many of them are no longer accessible.

image

@nzakas
Copy link
Member

nzakas commented May 21, 2025

I don't think all of the types from @eslint/core necessarily need to be exported from this package. People can just use @eslint/core directly for that.

@nzakas nzakas moved this from Needs Triage to Implementing in Triage May 21, 2025
@xbinaryx
Copy link
Author

Similar to the CSS and JSON plugins, I've modified the test to import SourceLocation and SourceRange from '@eslint/core'.

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.

LGTM, thanks. Would like another review before merging.

@nzakas nzakas moved this from Implementing to Second Review Needed in Triage May 22, 2025
Copy link
Contributor

@snitin315 snitin315 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!

Leaving it open for @lumirlumir

@lumirlumir
Copy link
Member

lumirlumir commented May 26, 2025

I’m not sure how other team members feel about this, but I’d like to point out that the current change introduces a Breaking Change.

For example,

In my custom Markdown plugin, I imported the ParserMode type from @eslint/markdown.

However, with this change, the ParserMode type is no longer exported directly from @eslint/markdown. ParserMode type now lives in dist/esm/language/markdown-language.d.ts, and the current package.json does not export that path.

As a result, users can no longer access it via:

  • @eslint/markdown
  • or even an alternative path like @eslint/markdown/esm/....

  • Current package.json export map:
  "exports": {
    ".": {
      "types": "./dist/esm/index.d.ts",
      "default": "./dist/esm/index.js"
    },
    "./types": {
      "types": "./dist/esm/types.d.ts"
    }
  },
  • Problem - parserMode type is no longer accessible:

image


But is this issue limited to just ParserMode type? I’d argue no.

As I mentioned in this earlier comment, some types were not explicitly exported from index.js.

However, thanks to Rollup’s behavior, the generated dist/esm/index.d.ts file exposed those types as if they were hoisted. This allowed users to import all types - even those not defined in index.js - directly from @eslint/markdown without any issues.

One such example is ParserMode. But other types may have been exposed in the same way, and if users were relying on them, this change could silently break their code.


While I agree that explicitly importing types from @eslint/core is a better and more intentional approach moving forward, introducing this change without clearly marking it as a breaking change could lead to confusion for users.

So, I'd like to ask the team to reconsider this issue.
If it's okay to keep this change without an explicit breaking change notice, I'm happy to follow that decision :)

Refernece: #367 (comment)

@nzakas
Copy link
Member

nzakas commented May 27, 2025

This is actually part of the reason I wanted to make this change. ParserMode is not intended to be exported, but due to the limitations with the way this package was written, it was exported. It's only used by MarkdownLanguage, which is not exported, so it's really just an internal type that should be of no use outside of the package.

I'm okay with marking this as a breaking bug fix if others agree.

@lumirlumir
Copy link
Member

lumirlumir commented May 28, 2025

I agree with marking this PR as a breaking change 👍

Could other team members share their thoughts on this? @eslint/eslint-team @eslint/eslint-tsc

@nzakas nzakas changed the title chore: remove rollup fix!: remove rollup and extraneous types May 28, 2025
@nzakas nzakas removed the bug label May 28, 2025
@nzakas
Copy link
Member

nzakas commented May 30, 2025

As a breaking change, we should hold off on merging this until we do another patch release to incorporate the non-breaking PRs that are in review currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Second Review Needed
Development

Successfully merging this pull request may close these issues.

Change Request: Stop rolling up package
4 participants