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!: bundle template files as 1 json file #32

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

Conversation

KSXGitHub
Copy link

This should allow the pnpm build script to stop copying the template files from this library to pnpm's dist assuming the bundle script used by pnpm can bundle JSON.

BREAKING CHANGE: getCompletionScript no longer returns a Promise

This should allow the pnpm build script to stop copying the template files
from this library to pnpm's `dist` assuming the bundle script used by pnpm
can bundle JSON.

BREAKING CHANGE: `getCompletionScript` no longer returns a `Promise`
@KSXGitHub KSXGitHub requested a review from zkochan October 1, 2024 12:09
@KSXGitHub KSXGitHub marked this pull request as ready for review October 1, 2024 12:10
@zkochan
Copy link
Member

zkochan commented Oct 1, 2024

What is the benefit of this approach?

@KSXGitHub
Copy link
Author

The location of the template files is an implementation details. Bundlers shouldn't need to know this location.

This PR would convert the template files from static assets that need to be manually read with fs and copied to the correct location during the bundling process into module files that can simply be required. This would allow bundlers to work with @pnpm/tabtab out of the box. The bundler would embed the template data directly in the resulting JavaScript file as an object of strings.

@zkochan
Copy link
Member

zkochan commented Oct 1, 2024

ok, but we already have this configured for pnpm or is this change for other consumers?

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.

2 participants