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

Port source from CoffeeScript over to Civet #47

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

Conversation

Mokshit06
Copy link

Related to #45.

Moves coffeescript source over to Civet and adds type definitions. Haven't removed the CS source code yet, as @danielx/civet doesn't have the unplugin plugin exported yet in the latest released npm version, so we can't publish it.

We can remove that once the type definitions and type names have been finalised and the build system can support the compilation from civet -> js + dts

Copy link
Owner

@edemaine edemaine left a comment

Choose a reason for hiding this comment

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

I've reviewed about half of the files so far. It's looking great! Here are some comments. I'll continue reviewing the other civet files (filter, oripa, viewer) that I haven't looked at yet.

I also turned on strict type checking, which has revealed a bunch of errors, but we can probably deal with many of them later...

I'm getting a problem with npm run build though:

> [email protected] build
> vite build

vite v4.5.0 building for production...

watching for file changes...

build started...
✓ 2 modules transformed.
Unexpected token (Note that you need plugins to import files that are not JavaScript)
file: C:\Users\edemaine\Projects\fold\src\convert.civet.tsx:1:10
1: var modulo: (a: number, b: number) => number = (a, b) => (a % b + b) % b;
             ^
2: import * as geom from "./geom.civet"
3: import * as filter from "./filter.civet"

Any idea why the type annotation didn't get removed there?

@@ -0,0 +1,24 @@
<!DOCTYPE html>
Copy link
Owner

Choose a reason for hiding this comment

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

I think I'd rather name this file foldviewer.html (as before). Maybe we eventually want other HTML files (e.g. docs) deployed too?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that sounds good.

examples: {
Default: 'examples/simple.fold',
'Flexicube Unit': 'examples/box.fold',
'Square Twist': 'examples/squaretwist.fold',
Copy link
Owner

Choose a reason for hiding this comment

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

Should we add cube-cp/cube-folded? (Makes me think it'd be cool to add to Fold Viewer an option like "unfold" which calls your new routine; and when edges_foldAngle is availabe, a "fold" button.)

"description": "FOLD file format for origami models, crease patterns, etc.",
"main": "lib/index.js",
"main": "dist/index.civet.js",
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder whether we should keep .civet in all the extensions. I think it'd be better to remove them. Is this easy to do, or does it break all the imports?

Copy link
Author

Choose a reason for hiding this comment

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

With the way the current unplugin works, it would break all the imports. Though we could maybe the file matcher for transpiled civet in it from "[path].civet.js" to "civet:[path].js", in which case the final extension should just be js. I'm not sure if it would make much of a difference though

"./oripa": {
"import": "./dist/oripa.civet.js",
"require": "./dist/oripa.civet.mjs"
}
Copy link
Owner

Choose a reason for hiding this comment

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

How do we expose types.civet?

Copy link
Author

Choose a reason for hiding this comment

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

We aren't generating dts files here yet, but once we should just be able to add types.civet as another entrypoint in vite config

edge
fold

export function unfoldedGeometry(fold: Fold, rootFace = 0)
Copy link
Owner

@edemaine edemaine Oct 28, 2023

Choose a reason for hiding this comment

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

There is a ton of shared code between these three functions. Can we factor them out into a generic function that gets called three different ways?

Also, it seems flatFoldedGeometry and flatUnfoldedGeometry are identical, so we don't need the latter (or could alias latter to former).

@Mokshit06
Copy link
Author

Any idea why the type annotation didn't get removed there?

Did you enable dts in civetPlugin? Vite & rollup plugins currently don't support dts generation, but I've got it working now, so will make a PR.

RE strict type checking, I think we should be adding runtime assertions rather than non null-assertions for fold property access

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.

Remove js from repo? Convert source from CoffeeScript to JavaScript or TypeScript or Civet?
2 participants