Support async compilers

remcohaszing commented 2 years ago

Initial checklist

[X] I read the support docs
[X] I read the contributing guide
[X] I agree to follow the code of conduct
[X] I searched issues and couldn’t find anything (or linked relevant results below)

Problem

Prettier 3.0.0-alpha.0 was just released. This makes all public APIs asynchronous.

remark-prettier uses those APIs in a unified compiler. It would be nice if this can keep working when it updates Prettier.

Solution

In addition to a string of buffer, a compiler may also return a promise that resolves to a string or buffer.

Of course just like with transformers, an async compiler means the unified pipeline no longer supports the synchronous methods.

Alternatives

remark-prettier could use deasync, but IMO that’s not a nice solution.

wooorm commented 2 years ago

deasync is indeed not nice.
I don’t see while serializing a tree, for unified, should ever be async
Should prettier really be run as a unified compiler? It doesn’t use the tree, which is the whole thing that unified provides. Why not run prettier afterwards?

JounQin commented 2 years ago

remark-prettier could use deasync, but IMO that’s not a nice solution.

You should at least use https://github.com/un-ts/synckit

eslint-mdx and many other eslint/prettier plugins are using it.

remcohaszing commented 2 years ago

remark-prettier offers the same thing that eslint-plugin-prettier offers: A single run (through remark CLI or language server) formats the code as desired, including Prettier formatting as well as other remark transforms.

Now that I’m thinking about it, this could also be resolved by unified-args and unified-language-server better supporting autofixes based on expected. This way no compiler is necessary.

JounQin commented 2 years ago

prettier@v3 will have a sync mode via worker_threads also: https://github.com/prettier/sync

But that is lack of features, yarn PnP support, for example: https://github.com/prettier/sync/issues/2

remcohaszing commented 2 years ago

@JounQin thanks for the hint! I’d still rather avoid forcing async code into synchronous, but synckit does look nicer than deasync because it doesn’t appear to use Node.js C++ bindings.

JounQin commented 2 years ago

eslint-plugin-prettier needs to adopt async version of prettier@v3 into sync due to ESLint limitation at the same time.

wooorm commented 2 years ago

@remcohaszing You have two plugins in one.

The first plugin assumes markdown is input (and that no plugin adds content to it). It passes that through prettier, fine. This could be useful as a plugin indeed. But it doesn’t depend on the AST.

So this could in my opinion better be a separate integration that accepts one (or more) vfiles, and accepts all the languages that prettier supports besides markdown, and generates lint messages for them.

The second plugin duplicates what remark-stringify does, and then does the same as what unified does with prettier: parse a file to a prettier tree, do things on the prettier tree, serialize the tree prettier. There is no reason to inline remark-stringify. And the same solution as the other plugin exists: a separate integration that accepts one (or more) vfiles, accepts all the languages that prettier supports besides markdown, and reformats them

remcohaszing commented 2 years ago

I agree this is basically two plugins in one and should be separated into two. And both of them don’t need to be specific to mdast.

The first plugin could compile the AST using the registered unified compiler and report diffs from the current VFile contents without depending on Prettier.

The second plugin could override the compiler, compile the AST using the old compiler, then reformat the code using Prettier. This way it would be language independant.

The second plugin would still require an async compiler. @JounQin’s suggestions really help though, so maybe it’s not needed.

wooorm commented 2 years ago

I don’t understand why they are done as a plugin.

This is how most tools work:

|     TS     |     babel   |    eslint    |    prettier   |
|            |             |              |               |
|    tree    |     tree    |     tree     |     tree      |
|   /    \   |    /    \   |    /    \    |    /    \     |
|  /      \  |   /      \  |   /      \   |   /      \    |
|string    string        string        string       string|

They can be glued together with Gulp or Grunt or bash scripts or whatever. They work on strings.

This is how unified works:

|                       unified                           |
|   parse    |  transform  |     check    |    format     |
|            |             |              |               |
|    tree -------- tree -------- tree --------- tree      |
|   /                                               \     |
|  /                                                 \    |
|string                                             string|

It works on trees. Some new things try to do the same, such as Rome. I believe this is the benefit that unified has over Gulp/Grunt/bash scripts. The reason it exists.

I don’t understand why this has to be solved in unified instead of:

import unified from 'unified'
import {prettier} from 'vfile-prettier';
import {reporter} from 'vfile-reporter'
import {read} from 'to-vfile'

const file = await unified()
  // ...
  .process(await read('readme.md'))

await prettier(file)

// Formatted content
console.log(String(file));

// Prettier formatting violations
console.error(reporter(file));

JounQin commented 2 years ago

@wooorm

remark-prettier is something just like eslint-plugin-prettier, it only requires a .remarkrc with

{
  "plugins": ["prettier"]
}

No code, just config.

wooorm commented 2 years ago

I understand what it does!

I don’t think it’s a good idea in unified itself.

There are differences between eslint and prettier and gulp and grunt etc and unified (as shown in the diagram above), which is why I don’t think it makes sense as a unified plugin

JounQin commented 2 years ago

Personally, I love the idea remark-prettier.

Because even with https://github.com/remarkjs/remark-preset-prettier, remark-stringify's default or few custom formatting can still change the input.

For example, there is no option to preserve two spaces:

Line.  <!-- two spaces here -->
Next line

It will always to formated as:

Line.\
Next line

This is very unexpected for prettier users.

remcohaszing commented 2 years ago

This is the same discussion as eslint-plugin-prettier vs prettier-eslint or stylelint-prettier vs prettier-stylelint. Some people prefer one solution, others prefer another, and that’s ok.

Anyway, I’ll close this issue, as @JounQin’s solution to use @prettier/sync will solve it. I will also split remark-prettier into two plugins as described in https://github.com/unifiedjs/unified/issues/196#issuecomment-1219241708 once Prettier 3 is officially released.

github-actions[bot] commented 2 years ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

github-actions[bot] commented 2 years ago

Hi team! Could you describe why this has been marked as wontfix?

Thanks, — bb

JounQin commented 2 years ago

I still want to discuss remark-stringify's formatting behavior. It is really unexpected to change unrelated codes when using remark-lint. 😂

Or maybe a new discussion/issue should be raised.

wooorm commented 2 years ago

This is the same discussion as eslint-plugin-prettier vs prettier-eslint or stylelint-prettier vs prettier-stylelint. Some people prefer one solution, others prefer another, and that’s ok.

Somewhat. The main thing for me is: should unified be Gulp? Gulp lets you stitch every tool together through wrapper plugins. I don’t believe so. Wrapper plugins are hard to maintain. They are always upgraded to new major versions later than the actual tool. And they often go unmaintained, with lots of bugs.

Regardless, Gulp like npm are task runners. They allow different tools to work together. That‘s a layer above unified. unified doesn’t have to focus on that.

To reiterate my previous diagrams, this is unified:

|                       unified                           |
|   parse    |  transform  |     check    |    format     |
|            |             |              |               |
|    tree -------- tree -------- tree --------- tree      |
|   /                                               \     |
|  /                                                 \    |
|string                                             string|

And this is gulp/npm scripts:

|     TS     |     babel   |    eslint    |    prettier   |
|            |             |              |               |
|    tree    |     tree    |     tree     |     tree      |
|   /    \   |    /    \   |    /    \    |    /    \     |
|  /      \  |   /      \  |   /      \   |   /      \    |
|string    string        string        string       string|

(and of course gulp/npm also support tools that don’t operate on ASTs and do other fancy things).

Lastly, unified is two things. It‘s this tiny package, and the whole ecosystem. There are different things that other packages can do, e.g., unified-engine depends on Node and is more of a “batteries included” thing. As this issue is here, me not thinking this issue fits here is one thing. Similar things make more sense in higher level projects.

saden1 commented 1 year ago

deasync is indeed not nice.

I don’t see while serializing a tree, for unified, should ever be async

Should prettier really be run as a unified compiler? It doesn’t use the tree, which is the whole thing that unified provides. Why not run prettier afterwards?

We live in a world we're many libs have to be async so as to not hang UI. MermaidJS is a good example. It's render function is async. You have to expect that some capabilities need to be async and if you ever introduce async anywhere in the code path and need to execute synchronously you are out of luck. I hope this helps.

It would be wise to allow for async plugins and would allow more innovation with unified.

saden1 commented 1 year ago

I don’t understand why they are done as a plugin.

This is how most tools work:


|     TS     |     babel   |    eslint    |    prettier   |

|            |             |              |               |

|    tree    |     tree    |     tree     |     tree      |

|   /    \   |    /    \   |    /    \    |    /    \     |

|  /      \  |   /      \  |   /      \   |   /      \    |

|string    string        string        string       string|

They can be glued together with Gulp or Grunt or bash scripts or whatever. They work on strings.

This is how unified works:


|                       unified                           |

|   parse    |  transform  |     check    |    format     |

|            |             |              |               |

|    tree -------- tree -------- tree --------- tree      |

|   /                                               \     |

|  /                                                 \    |

|string                                             string|

It works on trees.

Some new things try to do the same, such as Rome.

I believe this is the benefit that unified has over Gulp/Grunt/bash scripts.

The reason it exists.

I don’t understand why this has to be solved in unified instead of:


import unified from 'unified'

import {prettier} from 'vfile-prettier';

import {reporter} from 'vfile-reporter'

import {read} from 'to-vfile'

const file = await unified()

  // ...

  .process(await read('readme.md'))

await prettier(file)

// Formatted content

console.log(String(file));

// Prettier formatting violations

console.error(reporter(file));

You make a great and valid point with this example. One reason to use unified is to take full advantage of many plugins and not have one off solution. If everything is a plugin that increases reuse and allows flexibility in the ordering of plugin execution to perform transformation. Unified per its namesake should unify how things are done. I want to use mermaid, prism, and pettier using a unified and intuitive plugin api. I think that's reasonable. I am developing a web app and today I don't have the ability to use other tools to transform markdown documents and render mermaid, and highlight code, etc all in one shot.

wooorm commented 1 year ago

It would be wise to allow for async plugins and would allow more innovation with unified.

Plugins can be async: https://github.com/unifiedjs/unified#function-transformertree-file-next. You can use mermaid, prism in unified fine already.

This issue was about compilers, whose responsibility is ast -> string. I think that can be done sync. Can you show an example where that is not the case?

vlrevolution commented 8 months ago

It would be wise to allow for async plugins and would allow more innovation with unified.

Plugins can be async: https://github.com/unifiedjs/unified#function-transformertree-file-next. You can use mermaid, prism in unified fine already.

This issue was about compilers, whose responsibility is ast -> string. I think that can be done sync. Can you show an example where that is not the case?

It blocks the main thread in web apps tho?

wooorm commented 8 months ago

Then don’t do it on the main thread in web apps.

vlrevolution commented 8 months ago

I don't think it works out of box, tested yesterday, it complained about lacking document object/dom or something, in the web worker. I am completely new to web workers tho.

vlrevolution commented 8 months ago

This seems to cause an error because document is not available in the web worker: https://www.npmjs.com/package/decode-named-character-reference

ChristianMurphy commented 8 months ago

@vlrevolution that will depend on your framework and your build tool. Based on your comments on https://github.com/ssssota/svelte-exmarkdown/issues/209 and https://github.com/bytedance/bytemd/issues/35 you are using svelte with an unknown build tool. The answer would be svelte specific, you may want to use https://github.com/sveltejs/svelte/discussions/10738#discussioncomment-8732372 as inspiration.

ChristianMurphy commented 8 months ago

This seems to cause an error because document is not available in the web worker

Configure your build tool, a web worker version is provided, but you need to actually use it https://github.com/wooorm/decode-named-character-reference/blob/4347630741f82d7d4c7fb38763ba158951b2a1be/package.json#L36

vlrevolution commented 8 months ago

This seems to cause an error because document is not available in the web worker

Configure your build tool, a web worker version is provided, but you need to actually use it https://github.com/wooorm/decode-named-character-reference/blob/4347630741f82d7d4c7fb38763ba158951b2a1be/package.json#L36

I use vite for build. Thank you for the pointers tho, they help a lot!

What exactly does this mean? It seems kind of abstract to me, how to make vite use this worker version instead?

remcohaszing commented 8 months ago

Vite should respect the worker export condition when using web workers, but it doesn’t. See https://github.com/vitejs/vite/issues/7439

vlrevolution commented 8 months ago

Any way to override it?

ChristianMurphy commented 8 months ago

No, but there are other work arounds, see https://github.com/vitejs/vite/issues/7439 and try the suggestions there

vlrevolution commented 8 months ago

No, but there are other work arounds, see vitejs/vite#7439 and try the suggestions there

Many thanks, it is working!

For reference to others I put this in my vite.config.ts:

    resolve: {
        conditions: ['worker']
    },

unifiedjs / unified