un-ts / prettier

:package: Opinionated but Incredible Prettier plugins.
https://prettier.vercel.app
MIT License
260 stars 23 forks source link

Error with cjs version of prettier-plugin-sql `Cannot read properties of undefined (reading 'parse')` #332

Closed Brookke closed 6 months ago

Brookke commented 6 months ago

I am getting the following error when trying to use this plugin prettier on save in vscode:

TypeError: Cannot read properties of undefined (reading 'parse')
    at Object.print (/Users/user/Developer/app/node_modules/.pnpm/prettier-plugin-sql@0.17.1_prettier@3.1.1/node_modules/prettier-plugin-sql/lib/index.cjs:213:63)

I have established that the commonjs version of the plugin does not import JSOX correctly. The bundled code is as follows:

'use strict';

var jsox = require('jsox');
...
        let formatted = typeof value === "string" ? sqlFormatter.format(value, __spreadProps(__spreadValues({}, options), {
          params: params == null ? void 0 : jsox.JSOX.parse(
            params
          ),
          paramTypes: paramTypes == null ? void 0 : jsox.JSOX.parse(
            paramTypes
          )
        })) : parser.sqlify(value, { type, database });
     ...

Swapping, jsox.JSOX.parse to jsox.parse fixes the issue.

JounQin commented 6 months ago

Would you like to raise a PR which will be appreciated!

Brookke commented 6 months ago

Happy to help, however I don't have a proper fix yet other than directly modifying the bundled code. I think it might be a rollup config issue, I will spend some more time looking into how rollup is setup here

JounQin commented 6 months ago

https://github.com/un-ts/prettier/blob/3d85966479230654ebe34745966cc4fbb5d4213e/packages/sql/shim.d.ts#L2

It needs to be changed to

const JSOX = typeof JSON
export = JSOX

And the ESM version should be changed accordingly.

You can juat raise the PR and I'll review and guide to finish.

JounQin commented 6 months ago

https://github.com/d3x0r/JSOX/blob/44d2c55a1f0cfaf60f4b7f07f8565420f05498d0/package.jsox#L20-L24

https://github.com/d3x0r/JSOX/blob/44d2c55a1f0cfaf60f4b7f07f8565420f05498d0/lib/jsox.mjs#L3018

Interesting. This means it's an upstream issue actually...

cc @d3x0r

Brookke commented 6 months ago

https://github.com/un-ts/prettier/blob/3d85966479230654ebe34745966cc4fbb5d4213e/packages/sql/shim.d.ts#L2

It needs to be changed to


const JSOX = typeof JSON

export = JSOX

And the ESM version should be changed accordingly.

You can juat raise the PR and I'll review and guide to finish.

Yep I tried this locally when I was investigating. It seems like import { JSOX } from 'jsox' is valid and in CommonJS it's var jsox = require('jsox'). I assumed they'd be something in roll up that could fix this for us. But you're right ideally it'd be fixed upstream

JounQin commented 6 months ago

We need to support both of them at the same time.

// shim.d.ts
const JSOX: typeof JSON & { JSOX?: typeof JSON}

expot = JSOX

// index.ts
import * as _JSOX from 'jsox'

// workaround for inconsistent cjs/esm exports
const JSOX = _JSOX.JSOX || _JSOX
Brookke commented 6 months ago

Good idea, I'm happy to open a pr tomorrow with those changes (it's late UK time right now)