reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.7k stars 1.17k forks source link

rtk-query-codegen-openapi has outdated prettier 2.x peer dependency #4038

Closed FabianFrank closed 1 month ago

FabianFrank commented 9 months ago

Prettier 3 has been out for half a year, it is probably time to update, see https://prettier.io/blog/2023/07/05/3.0.0.html.

See also #3604

FabianFrank commented 9 months ago

Are there thoughts about removing the prettier dependency and support altogether and having people reformat the code after code gen? A dependency on specific prettier versions seems pretty rough as projects I assume are usually on their own prettier update schedule especially when it comes to major versions and 2.x to 3.x is a major breaking change for loading prettier plugins (prettier is now ESM).

phryneas commented 9 months ago

This honestly sounds like primarily a problem with your package manager - it should be standard procedure for your package manager to install prettier 2 in node_modules/@rtk-query/codegen-openapi/prettier (and for that to be used by the codegen) while your project uses prettier 3 from node_modules/prettier.

Did you enable some form of module hoisting?

But generally, this is an Open Source project maintained by volunteers in our spare time and we are open for Pull Requests.

FabianFrank commented 9 months ago

This honestly sounds like primarily a problem with your package manager - it should be standard procedure for your package manager to install prettier 2 in node_modules/@rtk-query/codegen-openapi/prettier (and for that to be used by the codegen) while your project uses prettier 3 from node_modules/prettier.

Did you enable some form of module hoisting?

I did suspect and investigate the package manager (npm in my case) angle at first as well, but it appears to me that npm behaves correctly and instead we are ending up with an incompatible combination of prettier and prettier plugins. I am using npm with default settings and from what I can observe it is working as intended, i.e. the @rtk-query/codegen-openai has its own 2.x version of prettier installed, which I verify as follows:

% jq < node_modules/prettier/package.json .version
"3.1.1"
% jq < node_modules/@rtk-query/codegen-openapi/node_modules/prettier/package.json .version
"2.8.8"

In addition there are also prettier plugins prettier-plugin-embed@0.3.2 and prettier-plugin-sql@0.18.0. These are prettier 3.x plugins and they thus can be (and are) ESM modules. Now when the CJS prettier@2.8.8 that ships with @rtk-query/codegen-openai loads the default prettier config that references the ESM module plugins it then fails to load them because you can't load ESM from CJS:

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/fabian/projects/brmlabs/brm/node_modules/prettier-plugin-embed/dist/index.js from /Users/fabian/projects/brmlabs/brm/node_modules/@rtk-query/codegen-openapi/node_modules/prettier/index.js not supported.
Instead change the require of /Users/fabian/projects/brmlabs/brm/node_modules/prettier-plugin-embed/dist/index.js in /Users/fabian/projects/brmlabs/brm/node_modules/@rtk-query/codegen-openapi/node_modules/prettier/index.js to a dynamic import() which is available in all CommonJS modules.
    at Module._extensions.<computed> [as .js] (/Users/fabian/projects/brmlabs/brm/node_modules/esbuild-runner/lib/hook.js:52:13)
    at /Users/fabian/projects/brmlabs/brm/node_modules/@rtk-query/codegen-openapi/node_modules/prettier/index.js:38143:10
    at Array.map (<anonymous>)
    at Object.load (/Users/fabian/projects/brmlabs/brm/node_modules/@rtk-query/codegen-openapi/node_modules/prettier/index.js:38141:128)
    at Object.load [as loadPlugins] (/Users/fabian/projects/brmlabs/brm/node_modules/@rtk-query/codegen-openapi/node_modules/prettier/index.js:16147:23) {
  code: 'ERR_REQUIRE_ESM'
}

But generally, this is an Open Source project maintained by volunteers in our spare time and we are open for Pull Requests.

I appreciate that and do like to contribute when possible. The reason I started this convo first is that I don't see a clear/obvious path forward between the following options:

  1. Remove the prettier dependency and related functionality from @rtk-query/codegen-openapi. Users have to call prettier themselves in their build scripts after running @rtk-query/codegen-openapi. Requires major version bump.
  2. Make prettier usage in @rtk-query/codegen-openapi configurable by adding a formatter?: "none" | "prettier" option that defaults to prettier for backwards compatibility. Requires minor version bump.
  3. Update prettier dependency in @rtk-query/codegen-openapi to 3.x. Users that can't accept the prettier update (for example potentially incompatible 2.x plugins) can only update @rtk-query/codegen-openapi after sorting their prettier update out. Requires major version bump (IMHO but maybe debatable?).

I personally prefer option 1 as it keeps @rtk-query/codegen-openapi focused on its core purpose and simpler with minimal downsides. Does that seem like something that could get merged or do you and the maintainers have other thoughts?

phryneas commented 9 months ago

I'd like to keep prettier in there as we have no way of reliably keeping the diff small if an upstream dependency changes, and the generated code might generally not be readable without that.

One thought - I believe this might be only problematic because we use the same prettier config as your main project:

https://github.com/reduxjs/redux-toolkit/blob/a10e10f7c28eec7594951dbf68a28af2a73d7779/packages/rtk-query-codegen-openapi/src/utils/prettier.ts#L29-L33

Could you try running this in a subfolder with an empty .prettierrc?

If that's all that is causing inconsistencies with parallel prettier 2&3 installations, we could allow to specify an additional prettier config in the codegen configfile. (Also, I believe the api didn't really change between prettier 2 and 3, so maybe we could also allow prettier ^2 || ^3 as a dependency, although I'm not sure if that would solve your ESM issues, as we still invoke prettier programmatically from a non-ESM-context.)

FabianFrank commented 9 months ago

I'd like to keep prettier in there as we have no way of reliably keeping the diff small if an upstream dependency changes, and the generated code might generally not be readable without that.

That makes sense, good point. 👍

One thought - I believe this might be only problematic because we use the same prettier config as your main project:

https://github.com/reduxjs/redux-toolkit/blob/a10e10f7c28eec7594951dbf68a28af2a73d7779/packages/rtk-query-codegen-openapi/src/utils/prettier.ts#L29-L33

Could you try running this in a subfolder with an empty .prettierrc?

Yes, that's definitely it. Running from a different folder with a .prettierrc that doesn't reference the plugins works.

If that's all that is causing inconsistencies with parallel prettier 2&3 installations, we could allow to specify an additional prettier config in the codegen configfile.

That should unblock projects that have a prettier config that is incompatible with the prettier that ships with @rtk-query/codegen-openapi because they can point at a minimal/empty/default prettierrc and reformat after using "their" prettier version and config if needed. It also wouldn't be a breaking change. I like that option.

(Also, I believe the api didn't really change between prettier 2 and 3, so maybe we could also allow prettier ^2 || ^3 as a dependency, although I'm not sure if that would solve your ESM issues, as we still invoke prettier programmatically from a non-ESM-context.)

You are right, allowing prettier 3.x would not solve the issue because @rtk-query/codegen-openapi is still CJS-only and thus can't load the ESM module plugins and we'd have to support running @rtk-query/codegen-openapi as ESM before you could fully share prettier config and plugins again.

phryneas commented 9 months ago

Would you consider trying a PR with that optional higher-priority prettier configuration?

That should unblock users like you, and then we can independently decide if we want to stay at prettier v2 for now or switch that to v3 at some point.

markerikson commented 1 month ago

Should be live in https://github.com/reduxjs/redux-toolkit/releases/tag/%40rtk-query%2Fcodegen-openapi%402.0.0-alpha.0 ! Please try it out and let us know if it works.