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

rewrite redot on top of ts-graphviz #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChristianMurphy
Copy link
Member

@ChristianMurphy ChristianMurphy commented Jul 28, 2023

  • 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

@ChristianMurphy ChristianMurphy mentioned this pull request Jul 28, 2023
9 tasks
@ChristianMurphy
Copy link
Member Author

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

@ChristianMurphy 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
@@ -19,14 +19,12 @@ npm install redot-parse
## Usage

```js
var unified = require("unified");
Copy link

@kamiazya kamiazya Jul 29, 2023

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)) 
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Comment on lines +3 to +5
import {args} from 'unified-args'
// eslint-disable-next-line import/order
import {redot} from 'redot'

Choose a reason for hiding this comment

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

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'

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ChristianMurphy ChristianMurphy Jul 29, 2023

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, will do

tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
Comment on lines +10 to +11
* @this {import('unified').Processor}
* @type {import('unified').Plugin<void[], import('ts-graphviz/ast').DotASTNode, string>}

Choose a reason for hiding this comment

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

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}

Copy link
Member Author

Choose a reason for hiding this comment

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

.npmrc Outdated Show resolved Hide resolved
Comment on lines +3 to +5
import {args} from 'unified-args'
// eslint-disable-next-line import/order
import {redot} from 'redot'
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Comment on lines +6 to +7
"main": "index.js",
"types": "index.d.ts",
Copy link
Member

Choose a reason for hiding this comment

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

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)

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

Choose a reason for hiding this comment

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

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
Copy link
Member

wooorm commented Jul 30, 2023

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

@ChristianMurphy
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🕸️ area/tests This affects tests ☂️ area/types This affects typings 🧑 semver/major This is a change
4 participants