Support async parser for embedded languages

[thorn0]

Description

Checklist

• I’ve added tests to confirm my change works.
• (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
• (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
• I’ve read the contributing guidelines.

✨Try the playground for this PR✨

[fisker]

What if we ask embed to return print function, so we don't need add an extra detectEmbeddedLanguage?

function embed() {
  const language = ...;
  return () => formatMarkdown(...)
}

[fisker]

And the logic don't need separate, and check language twice.

[thorn0]

Changing embed's return type from

Doc | undefined | Promise<Doc | undefined>

to

Doc | undefined | () => Promise<Doc | undefined>

is an interesting idea, but I've run out of time for now. :(

[fisker]

Take your time, no rush. There is no async parser in the world yet.

[thorn0]

I agree that this would be a better API.

[fisker]

thorn0#1

[fisker]

I have one more concern, if this get merged, the print can't be async. But some plugins call parse in print, and we already allow parse to be async, so the plugin will be broken. I guess the answer is move those parts to embed?

@prettier/plugin-pug format embed languages in this way.

//cc @Shinigami92

[thorn0]

I can't see where it calls parse in print

[fisker]

https://github.com/prettier/plugin-pug/blob/6ed005ec74e3ac2eb1a48699802b3f574f57998b/src/printer.ts#L650

[thorn0]

I guess that should be moved to embed then. Isn't this what embed is for?

[fisker]

As mentioned reason here, thorn0#1 (comment), this plugin has to move it to embed and add a fallback in print.

[fisker]

@Shinigami92 ^

Any comment?

[Shinigami92]

@Shinigami92 ^

Any comment?

Would make things much harder for me (I wrote the whole plugin in an iterative way instead of a AST walker because pug works much better and more performant this way)
But yeah, as this breaking change introduced by prettier I would assume a major version bump
and then I and other plugin maintainers need to deal with the breaking change 🤷

[fisker]

As I understand, you don't use print either, you traverse AST yourself?

In that case, you can also do print (async stuff) in parse... Some other plugins do this..

[thorn0]

@Shinigami92 Making printer.print async lead to a 20% slowdown. See #13158 (comment)

[Shinigami92]

I get the tokens in parse: https://github.com/prettier/plugin-pug/blob/6ed005ec74e3ac2eb1a48699802b3f574f57998b/src/index.ts#L69-L71
Pass it to print https://github.com/prettier/plugin-pug/blob/6ed005ec74e3ac2eb1a48699802b3f574f57998b/src/index.ts#L105-L107
And process it with a while-loop in the Printer: https://github.com/prettier/plugin-pug/blob/6ed005ec74e3ac2eb1a48699802b3f574f57998b/src/printer.ts#L325

[thorn0]

@fisker print can be allowed to be async if it's not used recursively (await only the root call), but that would be a really clumsy API. We might want to keep it undocumented.

[fisker]

We also have printer.preprocess() maybe should allow it to be async?

[sosukesuzuki]

Thanks @thorn0 and @fisker for the great work!

[fisker]

Will you try this?

[thorn0]

No, I really doubt it'll do a better job than ignoredProperties already does.

[thorn0]

How about moving printer.massageAstNode.ignoredProperties to printer.ignoredProperties? It also should be used in other places where the AST in traversed (e.g., hasNode).

[fisker]

If we are going to do this, I prefer

getVistorKeys(node) {}

Before we implement the real vistor keys, we can just simply use

const ignored = new Set(['tokens'])
getVistorKeys(node) {
	return Object.keys(node).filter(key => !ignored.has(key))
}

[thorn0]

That doesn't look performance-friendly. It will need to be called for each node: a function call, creating an array of keys, and iterating it. A simple set.has check must be much cheaper.

[fisker]

a function call, creating an array of keys, and iterating it.

When traverse ast, normally you need "get keys/values", "check keys", " iterating it", aren't they the same?

prettier/src/main/massage-ast.js

Lines 19 to 23 in 21b4045

	for (const [key, value] of Object.entries(ast)) {
	if (!ignoredProperties.has(key) && typeof value !== "function") {
	newObj[key] = massageAST(value, options, ast);
	}
	}

prettier/src/main/multiparser.js

Lines 77 to 88 in 21b4045

	for (const key in node) {
	if (
	Object.prototype.hasOwnProperty.call(node, key) &&
	!ignoredProperties?.has(key)
	) {
	if (Array.isArray(node[key])) {
	path.each(recurse, key);
	} else {
	path.call(recurse, key);
	}
	}
	}

[thorn0]

Yep. Somehow I misread what you're proposing. It might be not that bad.

[thorn0]

But why do you think a simple ignored set won't be good enough? Do you have any specific cases in mind?

[fisker]

Not really, just every estree traver I used use vistorKeys, maybe we can check what the reason behind it.

[fisker]

acorn add walk for each ast type from beginning, acornjs/acorn@b660ff6, I guess to be more effective?

[thorn0]

Probably. Also maybe it's important to visit keys in a specific order.

[fisker]

Many parsers come with traverse function, eg https://github.com/graphql/graphql-js/blob/67aefd9c1daefceacf937363c2da9e1117e550d9/src/language/visitor.ts#L177, if we remove AstPath someday, we may want use this function or vistorKeys.

[thorn0]

How are we going to know where we are at the tree without the path?
(Side question: do the tests pass locally for the next branch for you? I get 18 failed, seems to be related to color codes.)

[fisker]

Link to that I ask meriyah to supply such ithing meriyah/meriyah#180

[fisker]

I get 18 failed, seems to be related to color code

I found it few hours ago , was working on it, but late here. Caused by #13208. I changed cli test to not print color by default, but not working.

[thorn0]

Ah, okay. I'm glad that it's not something local with my env.
And another question: https://github.com/prettier/prettier/pull/13211/files#diff-f825b86383b78b8c50baa02327cfd46f6c1a3fee472409d978e5454672a83417R13
Is it expected that plugins persist across format calls? Looks like a bug to me.

[fisker]

How are we going to know where we are at the tree without the path?

Eslint use .parent #10785

[fisker]

Is it expected that plugins persist across format calls?

Not sure.

Maybe what I occurred in #13201 is related, but I forgot what the possible bug is, this "Revert plugin change to avoid possible bug" commit fixed test.

[thorn0]

Is it expected that plugins persist across format calls?

Tracked it down: #13235