feat: bump all upgradable deps, fix related usage #321
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| @@ -1,4 +1,5 @@ | |||
| { | |||
| "node": "14", | |||
There was a problem hiding this comment.
Interesting, is there a particular reason for node 14 over latest lts (16)?
There was a problem hiding this comment.
Right, and remains in active LTS until October of this year.
16 is the next LTS https://github.com/nodejs/Release#release-schedule and will be active LTS until October of next year, giving a bit more time for updates/fixes/support.
packages/eslint-mdx/src/traverse.ts
Outdated
| @@ -138,17 +138,20 @@ export class Traverse { | |||
| return | |||
| } | |||
|
|
|||
| let children = node.children as Node[] | |||
| let children = (node as Parent).children as Literal[] | |||
There was a problem hiding this comment.
this is probably fine, in other places I've been using
export function assertLiteral(node: Node) asserts node is Parent {
if ("children" in node) return
throw new TypeError("node is not a parent")
}or using https://github.com/syntax-tree/unist-util-assert
for some extra runtime/type safety, but it's up to you
There was a problem hiding this comment.
What type of node will has no value? Maybe it is worth adding a test case.
There was a problem hiding this comment.
A fair number of parent nodes don't have value.
https://github.com/syntax-tree/mdast#paragraph, https://github.com/syntax-tree/mdast#blockquote, https://github.com/syntax-tree/mdast#list, https://github.com/syntax-tree/mdast#listitem, https://github.com/syntax-tree/mdast#emphasis, etc
There was a problem hiding this comment.
Oh, I just remembered that we only care about ['export', 'import', 'jsx'] https://github.com/mdx-js/eslint-mdx/blob/master/packages/eslint-mdx/src/parser.ts#L38
Personally, I think Node should an optional value?: unkown, it will make the ts code much cleaner.
There was a problem hiding this comment.
Some context, the unist types are very bareboned, they are mainly used as interfaces for specific node types to extend, unified tries to make this easy through https://www.typescriptlang.org/docs/handbook/2/narrowing.html#discriminated-unions, where type is the discriminator which can differentiate nodes and their properties.
This is supported by https://github.com/syntax-tree/unist-util-visit-parents, https://github.com/syntax-tree/unist-util-visit, https://github.com/syntax-tree/unist-util-is, and https://github.com/syntax-tree/unist-util-assert which allow for quick and typesafe narrowing of types.
Or though handwritten predicates https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
Adding an index or extra properties to Node, Literal, and/or Parent breaks the discriminating union, because even after they type has been narrowed, you'd still have to check if a property really exists on this node type or not, which is something we want to avoid as much as possible.
There was a problem hiding this comment.
parentNode.value // this should error
Right, but it is still unknown
literalNode.children // this should also error
This is still an error with value?: unkown
This is what I mean balance. I know stricter types maybe better, but it is still maybe painful to write. Just my personal thought.
There was a problem hiding this comment.
I know stricter types maybe better, but it is still maybe painful to write. Just my personal thought.
We don't want the types to be painful. We may want the types to be opinionated.
Trying to identify nodes based off something other than type is a pattern we've seen beginners run into problems/bugs with.
And with that context, I'm okay with it being a bit painful in the typings by default, I want to steer people away from that pattern.
For people who have experience/expertise with typings, such as yourself, more advanced patterns may be useful, I'm cautiously okay with these project extending the base unist node types to support the project's needs (as long as it doesn't modify the global types, and impact other projects).
As a middle ground what would you think of:
- adding a TypeScript guide to https://unifiedjs.com/learn introducing common patterns in unified types
- I could pull down the project locally and open PR(s) suggesting places where ideomatic unified style type narrowing may simplify the code
Would you be interested in one or both?
There was a problem hiding this comment.
I'm OK with both, thanks if you want and can help to improve the types.
There was a problem hiding this comment.
I've created a draft of a TS guide at unifiedjs/unifiedjs.github.io#36, your thoughts and insights are welcome!
I'll pull down the latest code from eslint-mdx and take a look around soon.
There was a problem hiding this comment.
opened a version of what using concrete types for traversal could look like at #325 for further discussion
|
@mdx-js/releasers @wooorm Ready for a release. |
|
If I’m understanding this PR correctly, then some of @ChristianMurphy’s questions are still unanswered? 🤔 |
|
The types? That is not related to this PR very much as I understand. I drafted DefinitelyTyped/DefinitelyTyped#54413 for better typings. |
|
Are you planning on fixing the broken CI first before I publish? https://travis-ci.com/github/mdx-js/eslint-mdx |
I'll migrate to GitHub Actions soon, the test cases have been fixed locally. |
|
When is soon? |
Doing, just for a while. |
|
I believe you mean that you are currently working on it, and that it will take some time. |
|
Released! |
synckit is much faster now.