nene / prettier-plugin-sql-cst

Prettier SQL plugin that uses sql-parser-cst
https://nene.github.io/prettier-sql-playground/
GNU General Public License v3.0
123 stars 7 forks source link

Upgrade to prettier 3 #11

Closed nene closed 9 months ago

nene commented 11 months ago

This is now working, but it relies on the --experimental-vm-modules NodeJS flag, the use of which generates bunch of warnings when running the tests.

The good part is that (besides having to convert to async-await syntax) no tests needed to change. So this new Prettier 3 seems to format everything exactly as Prettier 2 did.

Perhaps there is a better way though. Should try out @prettier/sync which should eliminate the need to litter all tests with async-await. Maybe it also won't need this NodeJS flag.

nene commented 11 months ago

Experimented with @prettier/sync, but didn't get it working. Getting an error:

    DataCloneError: function (text, options) {
            return (0, transformCst_1.transformCst)((0, sql_parser_cst_1.parse)(text, {
    ...<omitted>... } could not be cloned.

Possibly related to @prettier/sync#4

karlhorky commented 9 months ago

it relies on the --experimental-vm-modules NodeJS flag, the use of which generates bunch of warnings when running the tests

In Node.js v21.3.0+ there is a --disable-warning flag (some signals it may be backported to v18 and v20)

karlhorky commented 9 months ago

Alternative

To avoid having to work with and maintain Prettier-specific code, the prettier-plugin-sql-cst project could change to a generic formatter library (eg. nene/sql-formatter-cst), for consumption by prettier-plugin-sql as it was already tried here (comment hidden and marked as offtopic):

And then prettier-plugin-sql could maintain the Prettier-specific parts.

JounQin commented 9 months ago

@prettier/sync is not full functional, you can use https://github.com/un-ts/synckit to wrap its async API directly very easily. See also https://github.com/prettier/prettier-synchronized/issues/2

nene commented 9 months ago

As I wrote in the linked issue, this plugin can't be separated from Prettier. The whole point of this plugin is to make use of Prettier to perform the actual code layout.

nene commented 9 months ago

@karlhorky thanks for the pointers though to other possible workarounds/solutions.

nene commented 9 months ago

@JounQin attempted to use synckit, but this didn't work out for me. Here's a PR #16 if you're interested.

In the end, I think the only win would have been a bit shorter syntax when writing tests and not having to convert all existing tests to async. But I did the latter already. On the downside I'd have more tooling around the tests - another thing that could break.

So I guess I'll live with the async stuff.

JounQin commented 9 months ago

OK, I'll help to find the cause when I'm free.


Keep async is good if prettier has already supported.

JounQin commented 9 months ago

@nene I take a look at the code base, --experimental-vm-modules is only required for jest so I don't think that's a big problem right? And this is a limitation by jest itself:

https://jestjs.io/docs/ecmascript-modules

And I suggest to migrate to Vitest instead which supports ESM very well.