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

Add Typescript definitions #34

Merged
merged 6 commits into from
Jul 30, 2023
Merged

Add Typescript definitions #34

merged 6 commits into from
Jul 30, 2023

Conversation

BearToCode
Copy link
Contributor

I added custom d.ts types definitions for the main entrypoints, closing #32.
I also changed the build process as following:

  • Add dist folder to .gitignore.
  • Add Rollup with a bunch of plugins to:
  • Add custom cjs build inside dist/node directory.
  • Add files: ["dist"] in package.json to only include build files in published package,

@matubu
Copy link
Member

matubu commented Jul 30, 2023

Thanks for your pull request! Looks great! 👍

@matubu matubu merged commit 6ad5536 into speed-highlight:main Jul 30, 2023
@matubu
Copy link
Member

matubu commented Jul 30, 2023

It looks like adding the dist folder to the gitignore has caused problems with the Deno package functionality. To resolve this, we may need to consider reversing that particular change.

Additionally, after merging the PR, the bundle size has increased significantly due to the dynamicImportVars plugin and the large switch case it creates. To make sure this doesn't negatively impact users who are not using Vite, we should work on separating the Vite version of the build from the main build.

@BearToCode
Copy link
Contributor Author

Regarding dynamicImportVars: if you prefer a smaller bundle size you can remove that plugin, but maybe add a mention of the issue in the readme so users can get a workaround.
Could you be more specific about the Deno problem? I might also look into that.

I also noticed a few options that need to be changed regarding types that I forgot to put in my previous PR, so you may want to wait so that I can add them in a new PR.

@BearToCode
Copy link
Contributor Author

@matubu I created a new PR for the fixes I mentioned (#35).

I think the problem with Deno is that the .zip file bundled in the GitHub release does not contain the dist folder. I did not understand how the release is created in the action, but I think that you to have to make the runner execute npm run build, zip the dist folder and use that in the release. (At least, that's my guess but you better check yourself)

@matubu
Copy link
Member

matubu commented Jul 30, 2023

Thanks for your PR, @BearToCode. I will work on fixing the deno package issue

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.

None yet

2 participants