🐈🐈🐈 #657

STRd6 · 2023-08-28T00:35:45Z

I scope creeped this to now allow for .civet in our Civet build.

Key ideas:

No checked in generated files to clutter up diffs
Allow us to use .civet as our source files when building and testing
Not substantially slower or requiring extra bootstrapping or downloads
It's how TS does it: a devDependency on a previously published stable version.

Also converted lib.ts -> lib.civet it uncovered several bugs but only needed minor changes. It is however a little slower (~5s) to parse and that is sad. Hopefully we can speed up the parser at it will be no worse than TS.

Fixes #641 Config update:

Removed config.civet from name of config files to check
If not found in .config dir keep looking rather than return null
Added some basic tests
Simpler dynamic import

edemaine

I still think having .civet as a format would be useful, partly because we don't (yet) support YAML, and Civet is a good approximation thereof. I agree it can only use the default config, but I think that's fine when all you're writing is

export default
  coffeeCompat: true

but I'd still rather write that than

{"coffeeCompat": true}

I'm hopeful that civetconfig.civet is a sufficiently specific name that it won't conflict with user's filenames... Probably 🐈.civet too. But we obviously do need to remove config.civet.

…e coverage reporting

edemaine · 2023-08-29T10:31:37Z

build/coffee-esm.mjs

@@ -3,9 +3,7 @@
 import { readFile } from 'fs/promises';
 import { pathToFileURL, fileURLToPath } from 'url';

-import CoffeeScript, { compile } from "coffeescript";
-// Handle cjs .coffee files
-CoffeeScript.register()


I think it's nice to keep this registration in case others want to use this as a CoffeeScript ESM handler. So I lean toward keeping it, but it's also OK to remove it and leave this for a CoffeeScript issue (actually jashkenas/coffeescript#5447).

edemaine · 2023-08-29T10:36:10Z

source/config.mts

@@ -0,0 +1,72 @@
+import path from "path";


Any particular reason for the move from CoffeeScript? ~~I preferred that version, and it's more consistent with the rest of the code (e.g. main.coffee).~~ Ah, I see, but we want to move toward full typechecking, so this makes sense.

edemaine · 2023-08-29T10:38:04Z

source/config.mts

+        "🐈.civet",
+        "civetconfig.json",
+        "civetconfig.civet",
+      ].includes(name)) {


name in ['config.civet', 'civet.json', 'civetconfig.civet', 'civetconfig.json'] has a more efficient compilation in CoffeeScript than this. But I guess we could do even better with a Set (hoisted to global scope).

edemaine · 2023-08-29T19:32:36Z

Wait, how can main be .civet? Isn't it needed to compile with the esbuild plugin?

STRd6 · 2023-08-29T19:45:13Z

Wait, how can main be .civet? Isn't it needed to compile with the esbuild plugin?

I added a devDependecy for a previously published civet version so we can us that in development as long as we reference it from node_modules.

edemaine

Oh right, I see it now. Amazing — we can now live the Civet future!!

Is it possible to switch lib.ts over too? Or is there an issue when mixing with Hera? That is where we could probably benefit most from Civet.

edemaine · 2023-08-29T20:42:03Z

source/config.civet

@@ -0,0 +1,72 @@
+import path from "path";


Not a high priority, but now that we're using Civet instead of TS, we could make this code closer to the CoffeeScript version you're replacing here (but with type annotations). Could be saved for the future though.

edemaine

So exciting!!! Civetception!

edemaine · 2023-08-30T15:52:22Z

package.json

      "./build/hera-esm.mjs",
-      "./dist/esm.mjs"
+      "./node_modules/@danielx/civet/dist/esm.mjs"


Will this use the correct version of Civet? We need tests to run in the repo version, not in the released version.

This is the version that will parse the source and test files. Once parsed the tests are run against main.civet, lib.civet, parser.hera, etc.

Ah, got it. (If only we could add a comment to a JSON file. 😉)

That seems fine, because we don't generally use fancy features in the test infrastructure, only inside the triple quotes. Actually, in some ways, it will be an improvement; I've run into issues where I break the Civet compiler (and ran yarn build) and then yarn test crashes, with unhelpful error messages. This should avoid that sort of issue.

edemaine · 2023-08-30T15:55:40Z

source/util.civet

@@ -225,7 +225,7 @@ export base64Encode = (src) ->

 # Accelerate VLQ decoding with a lookup table
 vlqTable = new Uint8Array(128)
-vlqChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/'
+vlqChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/';
 do ->


~~I don't understand how this do -> works without coffeeCompat mode enabled. Are we sure it's actually running?~~ Funny, there was already a coffeeCompat flag in a .coffee file. Is the following comment (at the top of the file) of interest?

# NOTE: Hera has an updated version of this file converted from CoffeeScript to Civet.

Yeah that directive was from some earlier testing.

We should probably export these utils from Hera as the definitive source eventually.

edemaine · 2023-08-30T16:46:33Z

package.json

@@ -43,15 +42,14 @@
    "@cspotcode/source-map-support": "^0.8.1"
  },
  "devDependencies": {
+    "@danielx/civet": "^0.6.26",


I'd feel safer if this were a pinned dep (no ^). Given how we tend to change things and just bump the patch number. 😅

I believe the lockfile should take care of it but being explicit here would also be fine.

edemaine · 2023-08-30T16:56:21Z

By the way, it's also really great what a stress test this was, for both JavaScript and CoffeeScript compatibility. Thanks for writing down all the bugs you encountered. Time for another bug squashing session! 😄

STRd6 · 2023-08-30T22:56:11Z

I'm still sad about the build getting slower with the conversion from lib.ts -> lib.civet but its not so bad as to hold this up.

We can try a few things in the future:

Optimization of the generated parser/grammar at the Hera level
Better optimized caching
Optimizations in the civet grammar / handlers
Convert to more idiomatic Civet in lib.civet (less code, fewer braces)

edemaine · 2023-08-31T11:49:37Z

All good ideas. And I'd rather pay the price of slowness and be able to dogfood. Civet should speed our development too.

One thing we might try is profiling the compilation stage. I have no idea, for example, whether the dominant cost is running Hera rules/caching, or the parser.hera code blocks (if we can distinguish these, maybe somewhat because most blocks now call lib), or the AST postprocessing we do. You may be right that it's the Hera parsing cost, but it also wouldn't surprise me if there is still some quadratic behavior in AST processing (like say calling addParentPointers at multiple recursive levels). Knowing which is dominant would help focus optimization.

STRd6 added 2 commits August 27, 2023 17:34

🐈.json

876d932

only json for now

9ef1a4b

edemaine reviewed Aug 28, 2023

View reviewed changes

STRd6 added 2 commits August 28, 2023 20:27

Added back some .civet config files; config.coffee -> config.mts; mor…

2fda1f7

…e coverage reporting

more config tests

9c68c69

STRd6 requested a review from edemaine August 29, 2023 04:43

edemaine approved these changes Aug 29, 2023

View reviewed changes

STRd6 added 3 commits August 29, 2023 09:06

🐈🐈🐈

7517e14

Merge branch 'main' into 🐈.json

02df0ea

fix build

4665c87

edemaine approved these changes Aug 29, 2023

View reviewed changes

STRd6 added 2 commits August 29, 2023 19:43

lib.ts -> lib.civet

6a980b2

civitify config.civet

efb2e06

STRd6 changed the title ~~🐈.json~~ 🐈🐈🐈 Aug 30, 2023

STRd6 requested a review from edemaine August 30, 2023 02:59

state-cache.civet

330cb36

edemaine approved these changes Aug 30, 2023

View reviewed changes

edemaine reviewed Aug 30, 2023

View reviewed changes

STRd6 merged commit 90daac5 into main Aug 30, 2023

STRd6 deleted the 🐈.json branch August 30, 2023 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐈🐈🐈 #657

🐈🐈🐈 #657

STRd6 commented Aug 28, 2023 •

edited

Loading

edemaine left a comment

edemaine Aug 29, 2023

edemaine Aug 29, 2023

edemaine Aug 29, 2023

edemaine commented Aug 29, 2023

STRd6 commented Aug 29, 2023

edemaine left a comment

edemaine Aug 29, 2023

edemaine left a comment

edemaine Aug 30, 2023

STRd6 Aug 30, 2023

edemaine Aug 30, 2023

edemaine Aug 30, 2023

STRd6 Aug 30, 2023

edemaine Aug 30, 2023

STRd6 Aug 30, 2023

edemaine commented Aug 30, 2023

STRd6 commented Aug 30, 2023

edemaine commented Aug 31, 2023

🐈🐈🐈 #657

🐈🐈🐈 #657

Conversation

STRd6 commented Aug 28, 2023 • edited Loading

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

edemaine commented Aug 29, 2023

STRd6 commented Aug 29, 2023

edemaine left a 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

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 commented Aug 30, 2023

STRd6 commented Aug 30, 2023

edemaine commented Aug 31, 2023

STRd6 commented Aug 28, 2023 •

edited

Loading