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

Bundle with tsup #670

Merged
merged 2 commits into from Nov 23, 2023
Merged

Bundle with tsup #670

merged 2 commits into from Nov 23, 2023

Conversation

nwalters512
Copy link
Contributor

This PR aims to resolve #604. It uses tsup to bundle code and emit both CJS and ESM (and dedicated types for each). While I don't love bundling code, tsup makes it really straightforward to get something that "just works".

An alternative we could use tsc directly to emit ESM/CJS builds to dist/esm and dist/cjs, respectively, then make a second pass with a custom script to rename dist/cjs/*.js to dist/cjs/*.cjs. Unfortunately tsc can't do this natively: microsoft/TypeScript#49462

"unpkg": "dist/sql-formatter.min.js",
"module": "lib/index.js",
"types": "lib/src/index.d.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the types field both here and in exports lets TypeScript resolve the correct types by filename.

@@ -3,16 +3,14 @@
"version": "14.0.0",
"description": "Format whitespace in a SQL query to make it more readable",
"license": "MIT",
"main": "dist/sql-formatter.min.cjs",
"main": "dist/index.cjs",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now uses the un-minified, non-UMD bundle. I think this is ok; unpkg will continue using the minified+UMD bundle produced by webpack.

// Intentionally use "export *" syntax here to make sure when adding a new SQL dialect
// we wouldn't forget to expose it in our public API.
export * from './allDialects.js';
// When adding a new dialect, be sure to add it to the list of exports below.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the export * was necessary because esbuild transformed it into code that wasn't statically analyzable for tree shaking. If desired, I can look into adding a test that asserts that this file has all the necessary language imports?

@nwalters512 nwalters512 marked this pull request as ready for review November 22, 2023 21:27
@nwalters512
Copy link
Contributor Author

@nene it looks like the latest version of tsup is expecting Node 18+. However, it looks like Node is pinned at v14 in CI. Are you willing to use a newer version of Node, or should I look into other solutions?

@nene
Copy link
Collaborator

nene commented Nov 23, 2023

No other reason of using Node 14 besides me not having bothered to witch the CI to a newer version.

@nene nene merged commit b2aceb8 into sql-formatter-org:master Nov 23, 2023
0 of 2 checks passed
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.

Package cannot be imported for certain TypeScript configurations
2 participants