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

Add support for printing Literal nodes #310

Closed
wants to merge 2 commits into from

Conversation

benjamingwynn
Copy link

@benjamingwynn benjamingwynn commented Sep 27, 2022

Fixes "Error: unknown node type: Literal" when using prettier.__debug.formatAST with an AST from svelte.parse.

Related: #211

Fixes "Error: unknown node type: Literal" when using _prettier.__debug.formatAST with an AST from svelte.parse.
@dummdidumm
Copy link
Member

Could you give more context? What is prettier.__debug.formatAST, why do you need it/call it, and what is the code that throws this error when invoking it? I'm hesitant to add fixes like this which doesn't seem to have any use for the regular formatting task. A test which fails when this isn't added would be nice, too.

@benjamingwynn
Copy link
Author

benjamingwynn commented Sep 28, 2022

@dummdidumm no worries, I'll try to elaborate.

Much like #211, we're asking Prettier to print using an AST which is generated from (svelte/compiler).parse() (https://svelte.dev/docs#compile-time-svelte-parse)

prettier.__debug.formatAST is not yet a public API, but there are future plans to expose this publicly, see prettier/prettier#9114. I imagine that although prettier-plugin-svelte doesn't need this functionality today, it will in the future.

A common alternative to using __debug.formatAST is creating a plugin that extends prettier-plugin-svelte and overriding parse() to return your AST, like so prettier/prettier#5998 (comment).

You still run into this issue with that approach, so #221 used astring at the default case of the switch block instead of handling the unknown nodes "properly", but we found only "Literal" was slipping through, hence this PR.

Interestingly, the "Literal" node is already referenced in expandNode() here:

https://github.com/sveltejs/prettier-plugin-svelte/blob/master/src/print/index.ts#L1154

So it seems to have been a consideration at some point. Both expandNode() and print() check if the type of the node is "Identifier" and handle it appropriately, so I don't see why the same isn't done for "Literal".

Happy to write a test case for this if you're happy with the justification.

@dummdidumm
Copy link
Member

dummdidumm commented Sep 28, 2022

I still don't understand how this related to some AST formatting methods in Prettier. "Literal" isn't a Svelte AST node, it's a AST node inside JS expressions (which are inside Svelte AST Mustache Tag nodes for example), which shouldn't appear at this code position. A test or at least a code snippet with some explanation why it fails would be very helpful for me to better understand what the problem is.

@benjamingwynn
Copy link
Author

I don't have time this evening to write a full test and add it to the PR, but hopefully this code snippet gives you an idea:

const prettier = require("prettier");
const sveltePrettier = require("prettier-plugin-svelte");
const svelteCompiler = require("svelte/compiler");
try {
  /** prettier config */
  const config = {
    tabWidth: 4,
    // ... etc
  };
  /** input source code */
  const src = "<some-component data-foo={123}>Hello world.</some-component>";

  // get ast from svelte compiler
  const ast = svelteCompiler.parse(src);
  console.log("[ast]", JSON.stringify(ast, null, 4));
  // ^ shows a node with type literal for 123

  // try to output the ast as code using prettier-plugin-svelte
  const out = prettier.__debug.formatAST(ast, {
    ...config,
    parser: "svelte",
    printer: sveltePrettier,
    plugins: [sveltePrettier],
    originalText: src,
  });
  // ^ falls over with "Error: unknown node type: undefined"

  console.log("[out.formatted]", out.formatted);
  // ^ should print the same as src

  console.error(
    out.formatted === src ? "[PASS] ✅" : "[FAIL] ❌ mismatched string"
  );
} catch (err) {
  console.error("[FAIL] ❌", err);
}

@dummdidumm
Copy link
Member

dummdidumm commented Sep 28, 2022

I have a hunch why this doesn't work: Do you need to pass all plugins that transform the code to formatAST, even the built-in Prettier plugins? If so, that's the problem: prettier-plugin-svelte uses the embed feature of Prettier, which means "print this section of the code with another plugin". In case of 123 it should be printed by the babel printer, which is built-in to Prettier. Other printers needed are babel-ts, typescript and css. The fix in our side would be to fall back to not formatting this at all in case the plugin isn't found.

@benjamingwynn
Copy link
Author

Hi, sorry for the late response. Your reply makes a lot of sense, but passing the parsers from Prettier manually results in the same error at the moment.

--- a/index.cjs
+++ b/index.cjs
@@ -1,5 +1,7 @@
 const prettier = require("prettier");
 const sveltePrettier = require("prettier-plugin-svelte");
+const __tsPrettier = require("prettier/parser-typescript");
+const __babelPrettier = require("prettier/parser-typescript");
 const svelteCompiler = require("svelte/compiler");
 try {
   /** prettier config */
@@ -20,7 +22,7 @@ try {
     ...config,
     parser: "svelte",
     printer: sveltePrettier,
-    plugins: [sveltePrettier],
+    plugins: [__tsPrettier, __babelPrettier, sveltePrettier],
     originalText: src,
   });
   // ^ falls over with "Error: unknown node type: undefined"
[FAIL] ❌ Error: unknown node type: undefined
    at Object.print (/Desktop/sample-for-pr/node_modules/prettier-plugin-svelte/plugin.js:1398:11)
    at callPluginPrintFunction (/Desktop/sample-for-pr/node_modules/prettier/index.js:8420:26)
    at mainPrintInternal (/Desktop/sample-for-pr/node_modules/prettier/index.js:8369:22)
    at mainPrint (/Desktop/sample-for-pr/node_modules/prettier/index.js:8356:18)
    at printAstToDoc (/Desktop/sample-for-pr/node_modules/prettier/index.js:8348:18)
    at formatAST (/Desktop/sample-for-pr/node_modules/prettier/index.js:8862:22)
    at Object.formatAST (/Desktop/sample-for-pr/node_modules/prettier/index.js:37229:12)
    at Object.<anonymous> (/Desktop/sample-for-pr/index.cjs:21:32)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)

Maybe embed() should be called in the default case of print() here?

@dummdidumm
Copy link
Member

mhm that sounds like a Prettier bug then, because Prettier states that it tries to call embed before print. What happens if you call embed at the top of print and return early if embed returns something - does it work then with the parsers? Does it work then without passing the additional parsers to formatAST?

@benjamingwynn
Copy link
Author

benjamingwynn commented Oct 11, 2022

I can't seem to figure out what I should pass as the third argument for textToDoc, but embed never seems to return anything. (e is always null)

function print(path, options, print) {
    const bracketSameLine = isBracketSameLine(options);
    const n = path.getValue();
    if (!n) {
        return '';
    }
    const e = embed(path, print);
    debugger;
    if (e) {
        return e;
    }

I just hacked the dist file for this quick patch as I didn't have to mess around with setting up the build, apologies if it doesn't match src exactly. n (the node) is {html, css, instance, module} (top-level), so I'm not sure if it should be walking through at this point or what.

mhm that sounds like a Prettier bug then

This is very likely in all honesty, stuff under the __debug namespace is probably not supposed to be used yet. That being said, the same thing occurs if you do the following hack to construct a plugin (from #211)

export function printCode(ast: Ast) {
    const overridePlugin = {
        ...p,
        parsers: {
            ...p.parsers,
            svelte: {
                ...p.parsers.svelte,
                parse: () => {
                    return { ...ast, __isRoot: true };
                },
            },
        },
    };
    return prettier.format(' dummy ', {
        parser: 'svelte',
        plugins: [overridePlugin as any],
    });
}

This works for #211 because they have a default in their switch which pipes unknown nodes through astring - see dc8ddb2#diff-fdd9b7cec43f9e7c596cd66d5e1656a3fc2ad2c0ae98cc7a165273a560a022e1R626

I think adding a default case which calls embed would work here, I just can't figure out what to do with that third argument, or if I should be calling prettiers api rather than the api of this plugin, I'm still learning my way around with Prettier stuff so you'll have to forgive me.

@benjamingwynn
Copy link
Author

Closing because printing Literal nodes specifically isn't valid. I imagine the update to prettier 3 will change how embed() internally has to be used, and may fix this issue by proxy

@benjaminpreiss
Copy link

I am having the same problem - can't hijack format to print the AST I am passing it... Do you folks have a clue why?

@benjamingwynn
Copy link
Author

I am having the same problem - can't hijack format to print the AST I am passing it... Do you folks have a clue why?

@benjaminpreiss i think prettier needs both originalText and an ast when doing prettier.__debug.formatAST. in our implementation we pass it as empty string ("").

furthermore this __debug.formatAST api doesnt call other languages from within it, so if you're processing a svelte file it wont be able to understand the javascript part of the AST. to fix this, you need to patch in Literal support into prettier-plugin-svelte

we had to solve various other problems with this manually, like indentation being wrong and losing comments

in conclusion, something like this:

import * as _sveltePrettier from './my-prettier-plugin-svelte-fork/esm/standalone.mjs';
import PRETTIER_CONFIG from './my-prettier-config.json';
return prettier.__debug.formatAST(ast, {
    ...PRETTIER_CONFIG,
    parser: 'svelte',
    plugins: [_sveltePrettier],
    printer: _sveltePrettier,
    originalText: '',
}).formatted;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants