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

Move unplugin and build using Civet instead of tsup #1306

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

edemaine
Copy link
Collaborator

@edemaine edemaine commented Jul 7, 2024

My original goal was to enable cli.civet to be imported from source (in particular because we don't ship it in dist), to enable Mocha testing. The issue was that the CLI depends on the unplugin, which lived in a completely different universe (integration/unplugin). This PR fixes this issue by moving the unplugin into source/unplugin, which makes sense given that the CLI depends directly on the unplugin (for typechecking). In particular, cli.civet now imports locally instead of relying on how it will be in dist:

diff --git a/source/cli.civet b/source/cli.civet
index 8399c99..f9bb554 100644
--- a/source/cli.civet
+++ b/source/cli.civet
@@ -2,7 +2,7 @@
 { findConfig, loadConfig } from ./config.civet

 // unplugin ends up getting installed in the same dist directory
-{rawPlugin} from ./unplugin
+{rawPlugin} from ./unplugin/unplugin.civet
 let unplugin:
   buildStart: () => Promise<void>
   buildEnd: (this: {emitFile: (data: {source: string, fileName: string, type: string}) => void}, useConfigFileNames?: boolean) => Promise<void>

Another benefit of this approach is that this import now gets the correct typing:

image

Before this PR we got this:

image


A secondary goal is that I wanted to start using Civet to write the unplugin, instead of being restricted to TypeScript. So the build system changed from raw tsup to an extension of our usual esbuild script, but now using the esbuild unplugin (from past versions) so that we can emitDeclarations and get automatic .d.ts files. This was a bit painful to get working, and build.sh has some hacks to work around some issues, but it seems to work, and may be a good model for future automatic .d.ts extraction. Notably, the only changes in the dist directories are as follows:

$ diff dist-old dist
diff dist-old/civet dist/civet
42c42
< var import_unplugin = require("./unplugin");
---
> var import_unplugin = require("./unplugin/unplugin.js");
diff dist-old/esm.mjs dist/esm.mjs
4c4
< import Civet from "./main.js";
---
> import Civet from "./main.mjs";

The second diff seems to have been a small bug: esm.mjs should import from main.mjs not main.js.

The first diff is because of one other change: I moved all the built unplugin files from dist to dist/unplugin. I think this is a bit more organized, and it makes the directory structure of dist match source, so relative imports still work (e.g. {rawPlugin} from ./unplugin/unplugin.civet from above). Assuming users use the exports defined in package.json, such as "@danielx/civet/vite", they shouldn't notice any difference from this re-organization.

I also relied less on bundling, so instead of a factored-out unplugin-shared.mjs which just worked for ESM, we have direct imports of ./unplugin.{js,mjs} in e.g. vite.js. We also only generate one .d.ts file (no more .d.mts), so only run TypeScript once; the updated exports field in package.json should correctly specify .d.ts as the type declarations for both CJS and ESM. Here's a before and after for the built unplugin files:

1,21c1,18
< -rwx------+ 1 edemaine None    387 Jul  7 12:12 astro.d.mts
< -rwx------+ 1 edemaine None    386 Jul  7 12:12 astro.d.ts
< -rwx------+ 1 edemaine None  19583 Jul  7 12:12 astro.js
< -rwx------+ 1 edemaine None    383 Jul  7 12:12 astro.mjs
< -rwx------+ 1 edemaine None    245 Jul  7 12:12 rollup.d.mts
< -rwx------+ 1 edemaine None    244 Jul  7 12:12 rollup.d.ts
< -rwx------+ 1 edemaine None  19358 Jul  7 12:12 rollup.js
< -rwx------+ 1 edemaine None    154 Jul  7 12:12 rollup.mjs
< -rwx------+ 1 edemaine None  17362 Jul  7 12:12 unplugin-shared.mjs
< -rwx------+ 1 edemaine None   1102 Jul  7 12:12 unplugin.d.mts
< -rwx------+ 1 edemaine None   1102 Jul  7 12:12 unplugin.d.ts
< -rwx------+ 1 edemaine None  19433 Jul  7 12:12 unplugin.js
< -rwx------+ 1 edemaine None    136 Jul  7 12:12 unplugin.mjs
< -rwx------+ 1 edemaine None    237 Jul  7 12:12 vite.d.mts
< -rwx------+ 1 edemaine None    236 Jul  7 12:12 vite.d.ts
< -rwx------+ 1 edemaine None  19342 Jul  7 12:12 vite.js
< -rwx------+ 1 edemaine None    146 Jul  7 12:12 vite.mjs
< -rwx------+ 1 edemaine None    201 Jul  7 12:12 webpack.d.mts
< -rwx------+ 1 edemaine None    200 Jul  7 12:12 webpack.d.ts
< -rwx------+ 1 edemaine None  19366 Jul  7 12:12 webpack.js
< -rwx------+ 1 edemaine None    158 Jul  7 12:12 webpack.mjs
---
> -rw----r--+ 1 edemaine None   326 Jul  7 16:35 astro.d.ts
> -rwx------+ 1 edemaine None  1982 Jul  7 16:35 astro.js
> -rwx------+ 1 edemaine None   470 Jul  7 16:35 astro.mjs
> -rw----r--+ 1 edemaine None   127 Jul  7 16:35 esbuild.d.ts
> -rwx------+ 1 edemaine None  1749 Jul  7 16:35 esbuild.js
> -rwx------+ 1 edemaine None   227 Jul  7 16:35 esbuild.mjs
> -rw----r--+ 1 edemaine None   154 Jul  7 16:35 rollup.d.ts
> -rwx------+ 1 edemaine None  1742 Jul  7 16:35 rollup.js
> -rwx------+ 1 edemaine None   223 Jul  7 16:35 rollup.mjs
> -rw----r--+ 1 edemaine None  1056 Jul  7 16:35 unplugin.d.ts
> -rwx------+ 1 edemaine None 20079 Jul  7 16:35 unplugin.js
> -rwx------+ 1 edemaine None 18031 Jul  7 16:35 unplugin.mjs
> -rw----r--+ 1 edemaine None   150 Jul  7 16:35 vite.d.ts
> -rwx------+ 1 edemaine None  1728 Jul  7 16:35 vite.js
> -rwx------+ 1 edemaine None   215 Jul  7 16:35 vite.mjs
> -rw----r--+ 1 edemaine None   124 Jul  7 16:35 webpack.d.ts
> -rwx------+ 1 edemaine None  1749 Jul  7 16:35 webpack.js
> -rwx------+ 1 edemaine None   227 Jul  7 16:35 webpack.mjs

I've only minimally converted the unplugin from TypeScript to Civet. But this should reap benefits in the future. And enable CLI testing.

Copy link
Contributor

@STRd6 STRd6 left a comment

Choose a reason for hiding this comment

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

👏 Thanks, this cleanup work is super useful and greatly appreciated!

@edemaine edemaine merged commit ecb7f19 into main Jul 8, 2024
3 of 4 checks passed
@edemaine edemaine deleted the unplugin-integrate branch July 8, 2024 13:52
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