Skip to content

feat: improve code splitting by introducing subpackage entry points #836

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

Closed

Conversation

ruijdacd
Copy link

@ruijdacd ruijdacd commented Mar 5, 2025

This PR introduces a solution outlined in this comment, which enables more granular control over the bundle output.

Current Problem

When utilizing the formatDialect API, the entire package is bundled together instead of just the specific dialect provided by the user. This results in unnecessary bloat in the bundle size.

To illustrate this issue, here is a simple reproduction of a Vite app. In this example, both format(SQL_QUERY, { language: "postgresql" }) and formatDialect(SQL_QUERY, { dialect: postgresql }) produce the same bundle size, demonstrating the problem clearly.

You can view the example here:

Copy link

codesandbox-ci bot commented Mar 5, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@nene
Copy link
Collaborator

nene commented Mar 5, 2025

Thanks for the pull request, but I'm not sure I understand what you are trying to achieve here and how.

You're referencing my own comment... in an already solved issue. I don't quite get it.

You seem to claim that it's possible to detect at compile time which strings are used as the language option and only include sources for those SQL dialects. I don't understand how that's really possible. Would you care to explain.

@ruijdacd
Copy link
Author

ruijdacd commented Mar 5, 2025

Thanks for the pull request, but I'm not sure I understand what you are trying to achieve here and how.

You're referencing my own comment... in an already solved issue. I don't quite get it.

You seem to claim that it's possible to detect at compile time which strings are used as the language option and only include sources for those SQL dialects. I don't understand how that's really possible. Would you care to explain.

I apologize for the lack of detail, but I noticed that tree-shaking wasn't functioning as intended, particularly in Vite. I attempted to use formatDialect(), but it ended up bundling everything, behaving similarly to format().

The only way I managed to achieve a smaller bundle was by separating formatDialect from format and offering sub-imports for each language.

Let me know your thoughts and if you have any feedback!

@ruijdacd
Copy link
Author

ruijdacd commented Mar 5, 2025

I've made some changes to the Stackblitz to separate the reproduction into separate pages to visualize the JS size differences better.

@nene
Copy link
Collaborator

nene commented Mar 5, 2025 via email

@ruijdacd
Copy link
Author

ruijdacd commented Mar 5, 2025

Yeah, totally! I didn't except this to get released quickly. I'd be happy to help out wherever needed

@nene
Copy link
Collaborator

nene commented Mar 16, 2025

So, I analyzed this thing and found out that this code splitting did work as expected in the 12.0.0 version where it was first introduced. It was still working in 14.0.0, but it broke in 15.0.0.

Not sure which of the changes from 14 to 15 might have caused it to break.

@nene
Copy link
Collaborator

nene commented Mar 16, 2025

So, the cause seems to be #670. That change alone was released as 14.1.0-beta.1.

Strangely there's a comment that says:

Removing the export * was necessary because esbuild transformed it into code that wasn't statically analyzable for tree shaking

However the end result seems to be the opposite. Hmm...

@nwalters512
Copy link
Contributor

@nene I opened #842 to resolve the code-splitting issue.

nene added a commit that referenced this pull request Mar 18, 2025
As reported in
#836 (comment),
my previous changes from #670 broke the ability for this package to be
tree-shaken.

This PR takes the other approach I had proposed in #604: it runs `tsc`
twice, once to produce a CJS build and once to produce an ESM one. It
writes a `package.json` file containing `{"type": "commonjs"}` to
`dist/cjs/package.json` so that the files in that directory are treated
as CJS.

Since #670 introduced an `exports` field, the internal changes to paths
shouldn't be visible to consumers, so this should be safe to release as
a patch or minor version. That said, I wouldn't blame you if you wanted
to release this as a major version to play it safe.
@nene
Copy link
Collaborator

nene commented Mar 18, 2025

@ruijdacd I've released 15.5.0-beta.2 with a fix for this issue. Would be nice if you could give it a try to see if it also fixes the problem for you.

@nene
Copy link
Collaborator

nene commented Mar 20, 2025

I'm closing this as the problem has been solved by different approach in 15.5.0.

@ruijdacd feel free to still get back to me if this turns out to not fix the problem for you.

@nene nene closed this Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants