Skip to content

Change Request: Stop rolling up package #374

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
1 task
nzakas opened this issue May 13, 2025 · 9 comments · May be fixed by #383
Open
1 task

Change Request: Stop rolling up package #374

nzakas opened this issue May 13, 2025 · 9 comments · May be fixed by #383

Comments

@nzakas
Copy link
Member

nzakas commented May 13, 2025

Environment

HEAD

What problem do you want to solve?

Right now, we're rolling up the files in this package for distribution, we requires us cleaning up duplicate type definitions. This is a lot of work and we're stuck with how we can reference types between files due to these limitations.

What do you think is the correct solution?

Stop rolling up the package. This isn't strictly necessary because it's an ESM-only package, and therefore, we're not getting much extra value from rolling things up.

We can remove Rollup and just have tsc emit both types and code to the dist directory. The end result for plugin users will be the same and it will allow us more flexibility with how we import and define types.

Participation

  • I am willing to submit a pull request for this change.

Additional comments

No response

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage May 13, 2025
@eslintbot eslintbot added this to Triage May 13, 2025
@xbinaryx
Copy link

xbinaryx commented May 13, 2025

I'll look into this if there is consensus within the team.

@lumirlumir lumirlumir moved this from Needs Triage to Triaging in Triage May 14, 2025
@lumirlumir
Copy link
Member

lumirlumir commented May 14, 2025

I believe this issue aligns with the one I’ve been working on: #367.

While the combination of rollup and dudupe-types is nice, but it introduces unnecessary complexity when working with types.

If this is accepted, it will make the ongoing migration to the @import syntax (#367) much easier.

So, 👍 to this! Let’s move forward.


But one thing I’m afraid of is: wouldn’t this be a breaking change?

@nzakas
Copy link
Member Author

nzakas commented May 14, 2025

But one thing I’m afraid of is: wouldn’t this be a breaking change?

I don't think so. What is it you think would break?

@lumirlumir
Copy link
Member

I'm just wondering —

If we remove rollup and rely only on tsc emit, is it still possible to put all type declarations into a single file like index.d.ts, as we did before?

Does the automatically generated index.d.ts file emitted by tsc include all type information like before?

@nzakas
Copy link
Member Author

nzakas commented May 15, 2025

When doing this with tsc, it will generate an index.d.ts file to go along with the index.js file. There will also be .d.ts files for each individual file, and index.d.ts will import the other .d.ts files as needed.

For a package user, this change is transparent.

@lumirlumir
Copy link
Member

Thank you for the explanation.

In that case, I suppose there won’t be any issues if we stop bundling the packages. 👍

@fasttime
Copy link
Member

Sounds good to remove the Rollup bundling step to simplify the build process. +1 from me.

@mdjermanovic
Copy link
Member

Sounds good to me too 👍

@snitin315
Copy link
Contributor

Marking this as accepted. @xbinaryx Would you like to send a PR?

@snitin315 snitin315 moved this from Triaging to Ready to Implement in Triage May 20, 2025
@xbinaryx xbinaryx linked a pull request May 20, 2025 that will close this issue
1 task
@lumirlumir lumirlumir moved this from Ready to Implement to Implementing in Triage May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Implementing
Development

Successfully merging a pull request may close this issue.

6 participants