Remove `detectEmbeddedLanguage` #1

fisker · 2022-08-02T11:58:17Z

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/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

src/main/multiparser.js

thorn0 · 2022-08-02T12:53:40Z

src/language-css/embed.js

-    return doc ? [doc, hardline] : "";
+    return async () => {
+      const doc = await printFrontMatter(node, textToDoc);
+      return doc ? [doc, hardline] : "";


Not related to these PRs, but returning "" from here means that the usual print function will be called for this node. I wonder if this is intended.

Yes, the usual print should be called. https://github.com/prettier/prettier/blob/765cae25522f582b71a79e7fb10417eb4612abe6/src/language-css/printer-postcss.js#L100, but better to return undefined/null as embed designed.

BTW the plugin should always handle all node types in print, even if embed return a fallback doc, because we have embeddedLanguageFormatting option to skip embed.

thorn0 · 2022-08-02T12:56:25Z

src/main/multiparser.js

-    printer.detectEmbeddedLanguage?.bind(printer) ?? (() => true);
  const { ignoredProperties } = printer.massageAstNode;

  const pathStacks = [];


This variable needs a better name. embedCallResults?

Yah, need more work, just testing if it works.

It looks good. One more name idea: embedThunks

How about this 5c1a383 😄

Doesn't look like it might work :)

As we're breakingly changing the signature of embed anyway (the textToDoc parameter becomes async), I think we should simply forbid sync embeds, i.e. the return type should become undefined | () => Promise<Doc | undefined>.

Corrently use sync embed in a few places.

https://github.com/fisker/prettier/blob/03db215c07c3013c0bc598c0cf7c57a7cd5c8230/src/language-html/embed.js#L365

https://github.com/fisker/prettier/blob/03db215c07c3013c0bc598c0cf7c57a7cd5c8230/src/language-html/embed.js#L379

They can be sync, but they shouldn't use print in that case (because it's print that can try to get things from embeds that haven't become available yet). Should we probably remove print from the parameters of embed and pass it only to the returned functions then?

they shouldn't use print in that case

When calling print in the returned function, it's still possible not settled? Visting siblings doc, for example.

Should we probably remove print

In that case, why not pass both print and textToDoc to the returned function, to prevent

const promise = textToDoc(); return () => promise

When calling print in the returned function, it's still possible not settled? Visting siblings doc, for example

I guess we should just say that it's a design limitation: print should be used only to print the current node's descendents.

In that case, why not pass both print and textToDoc

👍

fisker · 2022-08-02T13:10:47Z

We are calling hasPrettierIgnore in this function, maybe we can also print ignored node during this process.

thorn0 · 2022-08-02T13:20:13Z

We are calling hasPrettierIgnore in this function, maybe we can also print ignored node during this process.

I wanted to do that, then decided to keep this change simple.

fisker · 2022-08-02T13:29:01Z

In theory, args can be passed by parent node (before this change), right?

fisker · 2022-08-02T13:31:08Z

Also this https://github.com/prettier/prettier/blob/765cae25522f582b71a79e7fb10417eb4612abe6/src/language-js/print/template-literal.js#L127, but we'll have to ignore it.

thorn0 · 2022-08-02T13:31:50Z

Passed to where? printPrettierIgnoredNode doesn't use args, so it should be safe indeed to print ignored nodes together with embeds.

fisker · 2022-08-02T13:34:08Z

No, I mean embed

thorn0 · 2022-08-02T13:37:54Z

Embed doesn't take args too, so nothing to worry about here

thorn0 · 2022-08-03T12:22:57Z

Let's go back to 05cc6d5
I can't see any benifits in building the promise chain.

fisker · 2022-08-03T12:48:20Z

Have you benchmarked this?

thorn0 · 2022-08-03T13:02:07Z

Have you benchmarked this?

Not yet.

fisker · 2022-08-03T14:19:13Z

Pass options too? When checking languages, the printer may don't need that to make decision.

src/main/multiparser.js

thorn0 · 2022-08-03T14:24:58Z

Pass options too? When checking languages, the printer may don't need that to make decision.

We should treat options and path the same way then. If we pass options to returned functions, then let's pass path too. So that code like this would be possible:

if (language === "graphql") {
  return formatGraphql;
}

fisker · 2022-08-03T14:28:57Z

Does this order textToDoc, print, options, path make sense to you?

thorn0 · 2022-08-03T14:31:25Z

Does this order textToDoc, print, options, path make sense to you?

textToDoc, print, path, options seems to be slightly better. path is used more often than options.

src/language-js/embed/html.js

fisker · 2022-08-03T15:18:32Z

The isNode() you added can be replaced to return children keys, so we can use eslint-visitor-keys or @typescript-eslint/visitor-keys, maybe the traverse can be faster, but this can be done later.

thorn0 · 2022-08-03T15:27:30Z

but this can be done later

Yep. Let me bring my second PC to run benchmarks on it :)

src/main/multiparser.js

thorn0 · 2022-08-04T15:44:59Z

No changes in performance compared to prettier#13211:

  'node ./bench.mjs alt-async parallel' ran
    1.00 ± 0.01 times faster than 'node ./bench.mjs embed-async-function parallel'
    1.02 ± 0.04 times faster than 'node ./bench.mjs alt-async serial'
    1.02 ± 0.01 times faster than 'node ./bench.mjs embed-async-function serial'

Remove detectEmbeddedLanguage

b79b775

fisker mentioned this pull request Aug 2, 2022

Support async parser for embedded languages prettier/prettier#13211

Merged

4 tasks

thorn0 reviewed Aug 2, 2022

View reviewed changes

src/main/multiparser.js Outdated Show resolved Hide resolved

Simplify

05cc6d5

thorn0 reviewed Aug 2, 2022

View reviewed changes

Reuse node

08c7dd9

fisker force-pushed the embed-async-function branch from 03db215 to 08c7dd9 Compare August 3, 2022 12:38

try/catch printer.embed call

b8c1f19

Format

3624be0

fisker added 2 commits August 3, 2022 21:53

Missing return

7de45d4

Remove print and textToDoc parameter

c6e7cb9

thorn0 reviewed Aug 3, 2022

View reviewed changes

src/main/multiparser.js Outdated Show resolved Hide resolved

Pass options again, correct print

b186653

Pass textToDoc, print, path, options

05ca540

thorn0 reviewed Aug 3, 2022

View reviewed changes

src/language-js/embed/html.js Outdated Show resolved Hide resolved

thorn0 added 2 commits August 3, 2022 21:04

Format, microrefactor

c0c6eee

Rename variable

5609800

thorn0 reviewed Aug 4, 2022

View reviewed changes

src/main/multiparser.js Outdated Show resolved Hide resolved

fisker added 3 commits August 4, 2022 15:24

Remove try/catch

d99ce34

Move print related logic into the returned function

074b18f

Return undefined instead of empty doc

502baf6

fisker marked this pull request as ready for review August 4, 2022 16:18

thorn0 merged commit 0c5faa5 into thorn0:alt-async Aug 4, 2022

fisker deleted the embed-async-function branch August 4, 2022 16:27

Remove detectEmbeddedLanguage #1

Remove detectEmbeddedLanguage #1

Uh oh!

Conversation

fisker commented Aug 2, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker commented Aug 2, 2022

Uh oh!

thorn0 commented Aug 2, 2022

Uh oh!

fisker commented Aug 2, 2022

Uh oh!

fisker commented Aug 2, 2022

Uh oh!

thorn0 commented Aug 2, 2022

Uh oh!

fisker commented Aug 2, 2022

Uh oh!

thorn0 commented Aug 2, 2022

Uh oh!

thorn0 commented Aug 3, 2022

Uh oh!

fisker commented Aug 3, 2022

Uh oh!

thorn0 commented Aug 3, 2022

Uh oh!

fisker commented Aug 3, 2022

Uh oh!

Uh oh!

thorn0 commented Aug 3, 2022

Uh oh!

fisker commented Aug 3, 2022

Uh oh!

thorn0 commented Aug 3, 2022

Uh oh!

Uh oh!

fisker commented Aug 3, 2022

Uh oh!

thorn0 commented Aug 3, 2022

Uh oh!

Uh oh!

thorn0 commented Aug 4, 2022

Uh oh!

Remove `detectEmbeddedLanguage` #1

Remove `detectEmbeddedLanguage` #1

fisker Aug 4, 2022 •

edited

Loading

thorn0 Aug 3, 2022 •

edited

Loading

fisker Aug 3, 2022 •

edited

Loading

fisker Aug 3, 2022 •

edited

Loading

thorn0 Aug 3, 2022 •

edited

Loading