Add civet unplugin #632

Mokshit06 · 2023-08-15T03:43:42Z

This PR adds an unplugin for simultaneously create vite, rollup, esbuild, rspack and esbuild transformer for civet.

STRd6

Looks good!

Some possible areas for future work:

Update the Civet package build step so that people won't need to install a separate package and will be able to import any plugin directly.
Add some basic tests for each plugin.

Overall this is very helpful as is, thanks!

edemaine

This is so exciting! Mainly some docs comments, plus I wonder:

Should @danielx/civet be a peerDependency (maybe with * or >= version) instead? Otherwise, wouldn't we need to publish this plugin after every Civet update? Hmm, or maybe package managers deal with this properly when versions don't mismatch. Indeed, I see vite-plugin-civet uses a dependency not a peerDependency, and I haven't had issues with using newer versions. But we still might want * or >= version dependency here so it will default to the latest or whatever dependency is in the project root (I hope?).

Update the Civet package build step so that people won't need to install a separate package and will be able to import any plugin directly.

I think this is how it works already. Even peerDependencies seem to install by default though (despite not for a while).

Reading https://nodejs.org/en/blog/npm/peer-dependencies, it seems like peerDependency is the "right" thing here, but I'm not certain how it works with a regular dependency...

edemaine · 2023-08-15T16:00:45Z

integration/unplugin/README.md

+
+- `dts`: `unplugin-civet` also supports generating `.d.ts` type definition files from the civet source, which is useful for building libraries.
+- `outputExtension`: Output filename extension to use. Default: `.civet.jsx`, or `.civet.tsx` if `js` is `false`.
+- `js`: Whether to transpile to JS or TS.


Can you state the default here? I believe it's false, in which case should also rewrite the .civet.jsx default for outputExtension. (Should be .civet.tsx, unless js is true.)

Yeah that should be corrected. Though now that I think of it, maybe js should be true by default. Most users would be using these for building applications and not libraries and in that case preserving types doesn't help much. I'm open to other opinions on this

A default of js: true would break dts: true. We could make dts imply js: false, but honestly js: false is safer; some TypeScript features are not correctly implemented by Civet with js: true, and it just costs a bit of time to use already existing TS integration. So I think js: false is best.

We could give imply js based on the value of dts first,

const transpileToJS = options.dts || options.js;

in which case both options should work. My main concern was that rollup and webpack can't process typescript without additional plugins, so would need to set js: true to make the setup work, but if js: false has better typescript features then let's go with that default.

Maybe we can include the {js: true} in the README where we give recommended workflows for Webpack and Rollup?

Sure, we can do that

integration/unplugin/README.md

Mokshit06 · 2023-08-15T16:52:42Z

Thank you!

Update the Civet package build step so that people won't need to install a separate package and will be able to import any plugin directly.

I assume you're talking about building this and outputting to dist like done for esbuild plugin in build/build.sh?

Should @danielx/civet be a peerDependency

I thought that peer dependencies were not installed by default, didn't realise that changed in v7. If we update the Civet package and include it in @danielx/civet then we won't require it, but if not, then keeping it a peer dependency is probably better.

edemaine · 2023-08-15T17:08:28Z

Oh, sorry, now I understand Daniel's comment: make this part of the @danielx/civet NPM package. I feel like separately released NPM packages is more common here, but I imagine the extra JS here is pretty minimal so it might make sense (and be convenient) to just include it. This would mean a bunch of the exports mappings in package.json should go to the top level.

STRd6 · 2023-08-15T17:15:55Z

I assume you're talking about building this and outputting to dist like done for esbuild plugin in build/build.sh?

Yes, that's correct. Also updating the root Civet package.json to expose ./unplugin, ./vite, ./esbuild, etc. We could also replace the existing esbuild plugin that is already exported with this to cut down on duplication.

It will bulk up the published package a little bit compared to separate packages but should tree-shake out just fine thanks to the import mappings.

Let me know if you have any questions about the build step, it's currently manually building a few different files that end up in dist.

Thanks again!

edemaine · 2023-08-15T17:18:59Z

integration/unplugin/README.md

+- `dts`: `unplugin-civet` also supports generating `.d.ts` type definition files from the civet source, which is useful for building libraries. Default: `false`.
+- `outputExtension`: Output filename extension to use. Default: `.civet.tsx`, or uses `.civet.jsx` if `js` is `true`.
+- `js`: Whether to transpile to JS or TS. Default: `false`.
+- `transformOutput`: Adds a custom transformer over jsx/tsx code produced by `civet.compile`.


Minor point: It might be worth keeping the text that described the arguments to the function.

Mokshit06 · 2023-08-15T17:36:22Z

Should be now exported from @danielx/civet!

edemaine · 2023-08-15T18:43:01Z

I think tsup devDependency is missing. I get

'tsup' is not recognized as an internal or external command,

Mokshit06 · 2023-08-15T18:52:53Z

You have to run yarn add inside integration/unplugin as well

edemaine · 2023-08-15T19:02:58Z

But build is failing; see CI. Does Yarn support submodules so that yarn would recursively yarn in there? (I'd also like to switch to pnpm where this would be easy...) Otherwise it would need to be in the build script, but that seems bad.

Mokshit06 · 2023-08-15T19:10:31Z

Yarn does support workspaces, but for that the root package.json should have private: true, which can't be the case for @danielx/civet as it needs to be published. I think we need to add it to the build script

edemaine · 2023-08-15T19:27:57Z

That's too bad. Would it be better if there was no package.json within integration/unplugin? Maybe the script and dep should just move to the top level. I don't like the idea of running yarn install every build...

Mokshit06 · 2023-08-16T05:13:48Z

Hm so we could either move the dependencies up to the top level (shouldn't require moving the script), or we can do yarn add inside integration/unplugin on postinstall. That way whenever yarn add is ran on the top level, the unplugin deps should also be installed

edemaine · 2023-08-16T12:46:40Z

Either way seems workable to me, though I'm not sure for the motivation of a separate package.json if we're fully integrating. @STRd6 do you have a preference?

Mokshit06 · 2023-08-16T13:10:07Z

I think that's a fair point. Adding it to top level 👍

edemaine

Now there seems to be an infinite build loop, both for me and for CI:

yarn build                                                    (unplugin-civet $)
yarn run v1.22.18
$ bash ./build/build.sh
$ bash ./build/build.sh
$ bash ./build/build.sh
$ bash ./build/build.sh
$ bash ./build/build.sh

Maybe you're using a different version of Yarn? v1 is standard, not v2. Or maybe you didn't test. 😉

Because there's no longer a integration/unplugin/package.json, I believe yarn --cwd ./integration/unplugin build just runs the same script as yarn build. I think you want to run tsup instead. Also, can you add type checking to the top-level yarn test script?

Mokshit06 · 2023-08-19T13:14:52Z

Yeah I think I didn't test this change 😆 I have added a private package.json for integration/unplugin as calling tsup from root package.json required rewriting all paths in integration/unplugin/tsup.config.ts to be relative to the root rather than the unplugin directory.

edemaine · 2023-08-19T21:28:53Z

integration/unplugin/src/index.ts

@@ -1,4 +1,5 @@
 import { TransformResult, createUnplugin } from 'unplugin';
+// @ts-ignore


This doesn't seem to be necessary. Right?

Hm yeah it looks like it works without this as well. That's weird because earlier it was throwing a type error that @danielx/civet is not available to integration/unplugin

edemaine · 2023-08-19T21:29:44Z

package.json

@@ -54,7 +54,7 @@
    "docs:build": "yarn build && vitepress build civet.dev",
    "docs:preview": "yarn build && vitepress preview civet.dev",
    "prepublishOnly": "yarn build && yarn test",
-    "test": "c8 mocha"
+    "test": "c8 mocha && tsc"


@STRd6 This is going to interact with #638. But I guess we can always add to exclude until we're ready.

#638 is now merged. Can you merge main and test what happens now? We might need to exclude it for now.

edemaine · 2023-08-19T21:34:26Z

tsconfig.json

-    // "skipDefaultLibCheck": true,                      /* Skip type checking .d.ts files that are included with TypeScript. */
-    // "skipLibCheck": true                              /* Skip type checking all .d.ts files. */
+    "skipDefaultLibCheck": true,                      /* Skip type checking .d.ts files that are included with TypeScript. */
+    "skipLibCheck": true                              /* Skip type checking all .d.ts files. */


Is this needed, given that you're excluding dist directories? It would be better to leave it false if possible. Ah, I see, it turns off checking of node_modules, which currently fails (from missing @types perhaps?). Still, seems like this turns off a lot of actual type checking... though we could leave it for later. Same for skipDefaultLibCheck which seems risky.

This is needed because unplugin's dts imports webpack, vite, rollup, rspack, and esbuild out of which we don't have webpack and rspack required in Civet, and @typescript/vfs imports lz-string. So when typescript does type checking it errors out on these imports. We could also install types for these packages, or add them to dev deps instead

Adding dev dependencies for types sounds good.

Tried adding them to dev dependencies, but @rspack/core has a type error in it's declarations (imports a dts file that doesn't exist) and type checking fails because of it. Maybe we should turn on skipLibCheck for now? skipDefaultLibCheck won't be required

edemaine · 2023-08-19T21:38:00Z

build/build.sh

+# unplugin
+yarn --cwd ./integration/unplugin build
+cp ./integration/unplugin/dist/* ./dist
+


For Windows, I realize chmod a-x dist/*.*[jt]s would be helpful.

Also, what is this new dist/chunk-UZRN3RFA.mjs file? Presumably it's not desired?

For the esm build, tsup splits the common chunks into a separate file by default. As all the bundler entrypoints /esbuild, /rollup etc. import the central civetUnplugin it splits that into this chunk and imports it in them. Mostly helps keep the bundle size smaller as its not duplicated

Ah, that is nice. Can we choose a better name for that file? Given that there's only one, maybe a custom name rule would work?

We can choose a better name for it https://esbuild.github.io/api/#chunk-names, but i'm not sure if we should. There currently only exists one of these, but might be more than that in the future. I would suggest keeping it as is, or just turning off splitting

I think it would be nice to rename it for now; we can always change it if we need multiple chunks in the future (seems unlikely for the unplugin?). Maybe unplugin-shared?

edemaine

I realize the examples from your repo aren't here. Probably they should be, for manual testing and for documentation?

Also, a reminder to finish the last couple tweaks here — would be great to get this out!

Mokshit06 · 2023-08-29T16:13:20Z

Should be good to land now!

STRd6 · 2023-08-30T23:07:27Z

TS errors may be fixed by merging latest main now that our internals are in Civet.

…lugin-civet

STRd6

👍 Looks good!

@edemaine any further comments?

edemaine

Looks good to me! Just tried out building including some examples.

edemaine · 2023-08-31T18:15:41Z

types/node.d.ts

@@ -1,6 +1,6 @@
 // Add _compile to Module
 declare namespace NodeJS {
  interface Module {
-    _compile(content: string, filename: string)
+    _compile(content: string, filename: string): any;


I'd prefer : unknown

integration/unplugin/examples/rollup/package.json

edemaine · 2023-08-31T18:25:09Z

integration/unplugin/examples/astro-example/.gitignore

@@ -0,0 +1,21 @@
+# build output


I think this directory should be called astro not astro-example.

edemaine · 2023-08-31T18:27:37Z

yarn.lock

@@ -182,51 +182,101 @@
  resolved "https://registry.yarnpkg.com/@esbuild/android-arm64/-/android-arm64-0.17.19.tgz#bafb75234a5d3d1b690e7c2956a599345e84a2fd"
  integrity sha512-KBMWvEZooR7+kzY0BtbTQn0OAYY7CsiydT63pVEaPtVYF0hXbUaOyZog37DKxK7NF3XacBJOpYT4adIJh+avxA==

+"@esbuild/[email protected]":


yarn.lock might need updating; it changed when I ran yarn

Mokshit06 · 2023-08-31T19:46:41Z

Made the suggested changes!

Add civet unplugin

8fbc0c9

Mokshit06 temporarily deployed to build August 15, 2023 15:39 — with GitHub Actions Inactive

STRd6 approved these changes Aug 15, 2023

View reviewed changes

edemaine approved these changes Aug 15, 2023

View reviewed changes

Mokshit06 added 2 commits August 15, 2023 22:46

Update docs

891d990

Stop dts and js from being true at same time

79c6290

edemaine reviewed Aug 15, 2023

View reviewed changes

Mokshit06 temporarily deployed to build August 15, 2023 17:21 — with GitHub Actions Inactive

setup build for unplugin in root

94710fc

Mokshit06 had a problem deploying to build August 15, 2023 18:42 — with GitHub Actions Failure

Install unplugin-civet deps on build

815ae69

Mokshit06 temporarily deployed to build August 15, 2023 19:24 — with GitHub Actions Inactive

move unplugin deps to top level

eedee6e

Mokshit06 had a problem deploying to build August 18, 2023 20:11 — with GitHub Actions Failure

edemaine requested changes Aug 18, 2023

View reviewed changes

Mokshit06 added 3 commits August 19, 2023 18:41

webpack entrypoint

847e80c

ts update config

eb4bd13

fix recursive build

f377cd8

Mokshit06 temporarily deployed to build August 19, 2023 21:24 — with GitHub Actions Inactive

edemaine reviewed Aug 19, 2023

View reviewed changes

STRd6 mentioned this pull request Aug 24, 2023

How to use Civet in qwik #514

Open

edemaine reviewed Aug 27, 2023

View reviewed changes

fix a few bugs

f3ac1cc

add examples

2548ecd

Mokshit06 had a problem deploying to build August 30, 2023 04:07 — with GitHub Actions Failure

Merge branch 'main' of https://github.com/danielXMoore/Civet into unp…

b7207c5

…lugin-civet

Mokshit06 temporarily deployed to build August 31, 2023 15:30 — with GitHub Actions Inactive

Mokshit06 requested review from STRd6 and edemaine August 31, 2023 15:37

STRd6 approved these changes Aug 31, 2023

View reviewed changes

update examples import source

e918496

Mokshit06 temporarily deployed to build August 31, 2023 18:14 — with GitHub Actions Inactive

edemaine approved these changes Aug 31, 2023

View reviewed changes

edemaine reviewed Aug 31, 2023

View reviewed changes

Mokshit06 added 2 commits September 1, 2023 01:15

setup examples correctly

525508e

change any to unknown in type defs

a751684

Mokshit06 temporarily deployed to build August 31, 2023 19:52 — with GitHub Actions Inactive

edemaine merged commit 5abce0f into DanielXMoore:main Aug 31, 2023
2 checks passed

		@@ -1,4 +1,5 @@
		import { TransformResult, createUnplugin } from 'unplugin';
		// @ts-ignore

Add civet unplugin #632

Add civet unplugin #632

Conversation

Mokshit06 commented Aug 15, 2023

STRd6 left a comment

Choose a reason for hiding this comment

edemaine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mokshit06 commented Aug 15, 2023

edemaine commented Aug 15, 2023

STRd6 commented Aug 15, 2023

Choose a reason for hiding this comment

Mokshit06 commented Aug 15, 2023

edemaine commented Aug 15, 2023

Mokshit06 commented Aug 15, 2023

edemaine commented Aug 15, 2023

Mokshit06 commented Aug 15, 2023

edemaine commented Aug 15, 2023 • edited Loading

Mokshit06 commented Aug 16, 2023 • edited Loading

edemaine commented Aug 16, 2023

Mokshit06 commented Aug 16, 2023

edemaine left a comment

Choose a reason for hiding this comment

Mokshit06 commented Aug 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edemaine left a comment

Choose a reason for hiding this comment

Mokshit06 commented Aug 29, 2023

STRd6 commented Aug 30, 2023

STRd6 left a comment

Choose a reason for hiding this comment

edemaine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mokshit06 commented Aug 31, 2023

edemaine commented Aug 15, 2023 •

edited

Loading

Mokshit06 commented Aug 16, 2023 •

edited

Loading