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

Upgrade to prettier 3 #11

Merged
merged 14 commits into from
Dec 10, 2023
Merged

Upgrade to prettier 3 #11

merged 14 commits into from
Dec 10, 2023

Conversation

nene
Copy link
Owner

@nene nene commented Oct 12, 2023

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
Copy link
Owner Author

nene commented Oct 14, 2023

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
Copy link

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
Copy link

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
Copy link

JounQin commented Dec 3, 2023

@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 prettier/prettier-synchronized#2

@nene
Copy link
Owner Author

nene commented Dec 3, 2023

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
Copy link
Owner Author

nene commented Dec 3, 2023

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

@nene
Copy link
Owner Author

nene commented Dec 10, 2023

@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.

@nene nene marked this pull request as ready for review December 10, 2023 08:46
@nene nene merged commit b2c93ea into master Dec 10, 2023
1 check passed
@nene nene deleted the prettier-3.0.3 branch December 10, 2023 08:47
@JounQin
Copy link

JounQin commented Dec 10, 2023

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


Keep async is good if prettier has already supported.

@JounQin
Copy link

JounQin commented Dec 17, 2023

@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.

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