rewrite redot on top of ts-graphviz #25

ChristianMurphy · 2023-07-28T19:36:39Z

replace custom grammar and stringifier with ts-graphviz
use jsdoc based typescript
align with unified toolchain
- drop lerna in favor of npm workspaces
- replace custom eslint with xo
- use remark-preset-wooorm for markdown linting
- remove precommit hooks
upgrade unified version
add tests
upgrade CI
move common documentation (contributing, code of conduct, etc) to .github repo

TODO:

update documentation
- match unified format
- document ESM only
- include more examples
resolve remark-lint warnings
add new redot-svg or redot-stringify-svg package?

Replaces #22

Closes #1
Closes #2
Closes #3
Closes #4
Closes #5

packages/redot-stringify/index.js

ChristianMurphy · 2023-07-28T19:47:19Z

/cc @wooorm and @remcohaszing if you have a moment, another set of eyes to review would be appreciated

kamiazya · 2023-07-29T00:27:31Z

packages/redot-parse/readme.md

@@ -19,14 +19,12 @@ npm install redot-parse
 ## Usage

 ```js
-var unified = require("unified");


@ChristianMurphy Since this will change to ESModule, it is likely that ERR_REQUIRE_ESM exception may be raised when this code is executed.

I thought it would be better to modify the code in Useage and mention that it is not available from CommonJS format packages.

Alternatively, you could go for the Dual package format, which is more expensive but supports both ESModule and CommonJS.
In that case, however, it would be better to keep the documentation in ESModule format.

Yep, I'm in the process of updating the documentation.
Still work in progress, but a preview of what I have for this package:

Install

This package is ESM only.
In Node.js (version 12.20+, 14.14+, or 16.0+), install with [npm][]:

npm install redot-parse

In Deno with [esm.sh][esmsh]:

import redotParse from 'https://esm.sh/redot-parse@1'

In browsers with [esm.sh][esmsh]:

<script type="module"> import redotParse from 'https://esm.sh/redot-parse@1?bundle' </script>

Use

Say we have the following module example.js:

import {unified} from 'unified' import redotParse from 'redot-parse' import redotSvg from 'redot-svg' main() async function main() { const file = await unified() .use(redotParse) .use(redotSvg) .process('digraph {Hello -> World}') console.log(String(file)) }

I plan on publishing as ESM only (as most of unified does).
Dual packaging ESM and CJS has a bunch of problems around the dual package hazard and buggy/inconsistent handling of dual packaging by build tools.

Thanks!

I am relieved to hear that the documentation has been prepared.

I didn't know about the dual package hazard, so I learned a lot.

remcohaszing · 2023-07-29T10:56:56Z

packages/redot-cli/cli.js

+import {args} from 'unified-args'
+// eslint-disable-next-line import/order
+import {redot} from 'redot'


Doesn’t this work?

Suggested change

import {args} from 'unified-args'

// eslint-disable-next-line import/order

import {redot} from 'redot'

import {redot} from 'redot'

import {args} from 'unified-args'

The root of the issue is the newline between the import and require

With the original code or with your patch, the error is:

✖ 4:1 There should be no empty line between import groups import/order

I think this ESLint rule just isn’t very good, so I don’t worry about it

remcohaszing · 2023-07-29T10:59:30Z

packages/redot-cli/license


-Copyright (c) 2014 Andrei Kashcha
+Copyright (c) 2018 Christian Murphy


Shouldn’t we keep the original license instead? It’s ok to rename it though. Maybe the original license should be copied to all packages as well. I’m not sure who made what.

The diff is a bit misleading, the original file wasn't a license, it was a notice.
The redot-parse previously embedded a peg parser written by Andrei, but not published to npm.
To meet the terms of MIT, this notice file was added to the project.
Andrei did not create redot project nor actively contributed to the project.
With the peg file removed there is no reason to keep the notice for the peg file.

remcohaszing · 2023-07-29T11:00:48Z

packages/redot-parse/index.js

@@ -0,0 +1,17 @@
+// eslint-disable-next-line n/file-extension-in-import


We use TypeScript for this check. IMO this rule can be disabled project-wide.

Good idea, will do

tsconfig.json

remcohaszing · 2023-07-29T11:08:39Z

packages/redot-stringify/index.js

+ * @this {import('unified').Processor}
+ * @type {import('unified').Plugin<void[], import('ts-graphviz/ast').DotASTNode, string>}


I think this may help

Suggested change

* @this {import('unified').Processor}

* @type {import('unified').Plugin<void[], import('ts-graphviz/ast').DotASTNode, string>}

* @this {import('unified').Processor<import('ts-graphviz/ast').DotASTNode, import('ts-graphviz/ast').DotASTNode, import('ts-graphviz/ast').DotASTNode>}

* @returns {undefined}

Thanks! I'll give this a try, but might need to do something like: https://github.com/remarkjs/remark/blob/26dc58a675ac7267c105f0fdb76a82db77f8402a/packages/remark-stringify/index.d.ts#L1-L9 in the end

.npmrc

wooorm · 2023-07-29T16:34:20Z

packages/redot-cli/cli.js

+import {args} from 'unified-args'
+// eslint-disable-next-line import/order
+import {redot} from 'redot'


I think this ESLint rule just isn’t very good, so I don’t worry about it

wooorm · 2023-07-29T16:37:02Z

packages/redot-cli/tsconfig.json

@@ -0,0 +1,3 @@
+{
+  "extends": "../../tsconfig.json"
+}


Doesn’t this need a references for TS to work?
It took me a bit to get it right but with this change the process becomes much faster than with the separate tscs we used to have in places: remarkjs/remark@f6bd64e

Thanks for the pointer,
It works, but it may not be as fast as it could be.
I aimed to match this with remark, but may have missed some config, I'll double check it.

Basically if you add a reference for each dependency inside the workspace, the build can be significantly sped up. However, cleaning the cache before building works against this.

It could be even faster by making a TypeScript project (e.g. the tsconfig.json file in the workspace root) that excludes all files, but references all workspaces. Then instead of running tsc in each workspace, running tsc in the project root.

wooorm · 2023-07-29T16:38:16Z

packages/redot-parse/index.js

+
+/** @type {import('unified').ParserFunction<import('ts-graphviz/ast').DotASTNode>} */
+function parser(document, vfile) {
+  return parse(document, {startRule: 'Dot', filename: vfile.path})


Q: should there be configuration?
Q: is DotASTNode a unist compatible node? Shouldn’t it subclass the same way types/hast or so works based on types/unist?

Q: should there be configuration?

Potentially? I could do a pass through of the ts-graphviz options.

Q: is DotASTNode a unist compatible node?

It is, it uses type and children the same as unist specifies.

Shouldn’t it subclass the same way types/hast or so works based on types/unist?

Maybe? What would be the benefit of subclassing here? Strong types on data? (I thought that was going away)

packages/redot-parse/test.js

wooorm · 2023-07-29T16:43:08Z

packages/redot-stringify/package.json

+  "main": "index.js",
+  "types": "index.d.ts",


Suggested change

"main": "index.js",

"types": "index.d.ts",

"exports": "./index.js",

Export map hides files for users, which is easier to document and prevent internal things from leaking.
With that, types and main is no longer needed (except when the .js -> .d.ts doesn‘t work)

wooorm · 2023-07-29T16:44:02Z

packages/redot-stringify/test.js

+import {test} from 'node:test'
+import * as assert from 'node:assert/strict'
+import {unified} from 'unified'
+import redotStringify from './index.js'


Suggested change

import redotStringify from './index.js'

import redotStringify from 'redot-stringify'

With export maps, you cal also use the package itself in tests. Making them closer to what users have to do!

* replace custom grammar with ts-graphviz * use jsdoc based typescript * align with unified toolchain * upgrade unified version * add tests * upgrade CI

wooorm · 2023-07-30T14:51:19Z

let me know when you need a another review, perhaps when docs and such are done?

ChristianMurphy · 2023-07-31T15:18:49Z

let me know when you need a another review, perhaps when docs and such are done?

Will do, I'll ping again after I've completed the updates

ChristianMurphy mentioned this pull request Jul 28, 2023

[WIP] Version 1.0 rewrite #22

Closed

9 tasks

ChristianMurphy commented Jul 28, 2023

View reviewed changes

packages/redot-stringify/index.js Outdated Show resolved Hide resolved

ChristianMurphy force-pushed the rewrite branch 2 times, most recently from af71d1d to a44f0cb Compare July 28, 2023 19:42

ChristianMurphy force-pushed the rewrite branch from a44f0cb to ea08857 Compare July 28, 2023 19:51

ChristianMurphy added 🧑 semver/major This is a change 🗄 area/interface This affects the public interface 🕸️ area/tests This affects tests ☂️ area/types This affects typings labels Jul 28, 2023

ChristianMurphy force-pushed the rewrite branch 7 times, most recently from 5ebbd9b to bfc71ca Compare July 28, 2023 20:31

kamiazya reviewed Jul 29, 2023

View reviewed changes

ChristianMurphy force-pushed the rewrite branch from bfc71ca to 81b8396 Compare July 29, 2023 02:13

remcohaszing reviewed Jul 29, 2023

View reviewed changes

ChristianMurphy force-pushed the rewrite branch from 81b8396 to bf6f787 Compare July 29, 2023 15:01

wooorm reviewed Jul 29, 2023

View reviewed changes

ChristianMurphy force-pushed the rewrite branch from bf6f787 to 4a160ab Compare July 29, 2023 20:06

rewrite redot on top of ts-graphviz

50a540c

* replace custom grammar with ts-graphviz * use jsdoc based typescript * align with unified toolchain * upgrade unified version * add tests * upgrade CI

ChristianMurphy force-pushed the rewrite branch from 4a160ab to 50a540c Compare July 29, 2023 20:32

ChristianMurphy mentioned this pull request Nov 15, 2023

Fix .indexOf() call on integers when using numbers as node id's #23

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewrite redot on top of ts-graphviz #25

rewrite redot on top of ts-graphviz #25

ChristianMurphy commented Jul 28, 2023 •

edited

ChristianMurphy commented Jul 28, 2023

kamiazya Jul 29, 2023 •

edited

ChristianMurphy Jul 29, 2023

ChristianMurphy Jul 29, 2023

kamiazya Jul 29, 2023

remcohaszing Jul 29, 2023

ChristianMurphy Jul 29, 2023

wooorm Jul 29, 2023

remcohaszing Jul 29, 2023

ChristianMurphy Jul 29, 2023 •

edited

remcohaszing Jul 29, 2023

ChristianMurphy Jul 29, 2023

remcohaszing Jul 29, 2023

ChristianMurphy Jul 29, 2023

wooorm Jul 29, 2023

wooorm Jul 29, 2023

ChristianMurphy Jul 29, 2023

remcohaszing Jul 31, 2023

wooorm Jul 29, 2023

ChristianMurphy Jul 29, 2023

wooorm Jul 29, 2023

wooorm Jul 29, 2023

wooorm commented Jul 30, 2023

ChristianMurphy commented Jul 31, 2023


		Copyright (c) 2014 Andrei Kashcha
		Copyright (c) 2018 Christian Murphy

		@@ -0,0 +1,17 @@
		// eslint-disable-next-line n/file-extension-in-import

		* @this {import('unified').Processor}
		* @type {import('unified').Plugin<void[], import('ts-graphviz/ast').DotASTNode, string>}

	"main": "index.js",
	"types": "index.d.ts",
	"exports": "./index.js",

	import redotStringify from './index.js'
	import redotStringify from 'redot-stringify'

rewrite redot on top of ts-graphviz #25

Are you sure you want to change the base?

rewrite redot on top of ts-graphviz #25

Conversation

ChristianMurphy commented Jul 28, 2023 • edited

ChristianMurphy commented Jul 28, 2023

kamiazya Jul 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Install

Use

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

ChristianMurphy Jul 29, 2023 • edited

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

wooorm commented Jul 30, 2023

ChristianMurphy commented Jul 31, 2023

ChristianMurphy commented Jul 28, 2023 •

edited

kamiazya Jul 29, 2023 •

edited

ChristianMurphy Jul 29, 2023 •

edited