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

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

Closed
FabianFrank opened this issue Jan 4, 2024 · 7 comments
Closed

Comments

@FabianFrank
Copy link
Contributor

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

FabianFrank commented Jan 4, 2024

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

phryneas commented Jan 4, 2024

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

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

phryneas commented Jan 4, 2024

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:

config = await prettier.resolveConfig(process.cwd(), {
useCache: true,
editorconfig: true,
});
}

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

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:

config = await prettier.resolveConfig(process.cwd(), {
useCache: true,
editorconfig: true,
});
}

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

phryneas commented Jan 4, 2024

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.

FabianFrank added a commit to FabianFrank/redux-toolkit that referenced this issue Jan 5, 2024
Allow users to optionally provide the explicit location of the prettier
config to use. For example unblocks users that are using an
incompatible/different major version of prettier as part of their
project that breaks the prettier version rtk query code gen depends on.

see reduxjs#4038
markerikson pushed a commit to FabianFrank/redux-toolkit that referenced this issue Aug 30, 2024
Allow users to optionally provide the explicit location of the prettier
config to use. For example unblocks users that are using an
incompatible/different major version of prettier as part of their
project that breaks the prettier version rtk query code gen depends on.

see reduxjs#4038
@markerikson
Copy link
Collaborator

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.

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

No branches or pull requests

3 participants