Skip to content

Move RTK-Query OpenAPI Codegen into Monorepo, prepare release 1.0 #1680

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

Merged
merged 110 commits into from
Nov 27, 2021

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Nov 2, 2021

This pretty much completely overhauls the way the Codegen is used, as suggested in rtk-incubator/rtk-query-codegen#63

Could some people please give this a try and report back?

The new README is here: https://github.com/reduxjs/redux-toolkit/blob/rtk-query-codegen-openapi/packages/rtk-query-codegen-openapi/README.md

Please note that right now you will have to use the CI build, so call

npx @rtk-query/codegen-openapi@https://pkg.csb.dev/reduxjs/redux-toolkit/commit/4b8f6b4f/@rtk-query/codegen-openapi/_pkg.tgz <configfile>

Where 4b8f6b4f should reference the latest commit.

phryneas and others added 30 commits December 6, 2020 19:27
RTK Query requires method property, so add it.
* Add a generator for react hooks option
* Add prettier, parse URLs and files from CLI

Co-authored-by: kahirokunn <[email protected]>
* Add chalk, support named and default exports from a baseQuery CLI arg
* Add jest, msw and CLI tests
* Add test for --file option
* Simple string snapshot serializer

Co-authored-by: kahirokunn <[email protected]>
Co-authored-by: Lenz Weber <[email protected]>
* start extracting ts-generation code

* add csb
@Lokaltog
Copy link
Contributor

Lokaltog commented Nov 9, 2021

Great! I've resolved conflicts in rtk-incubator/rtk-query-codegen#82 and submitted #1712 with @lindapaiste's changes. I'll look into the other issues later today.

@phryneas
Copy link
Member Author

phryneas commented Nov 9, 2021

Thanks!

Also, one last thing for the list:

  • extend README and extend Documentation on the Homepage (or at least link the new README)

@Lokaltog
Copy link
Contributor

Lokaltog commented Nov 9, 2021

@grumpyTofu @phryneas I've attempted to reproduce the ts-node issue but on my setup both ts-node and esr works fine. I've installed the latest CI package and tested it stock, and I've tested by commenting out the esr import in cli.js (forcing it to use ts-node) but it works fine. TS is already configured to output CJS modules so I'm not sure what's wrong here.

@grumpyTofu could you provide a repo with a clean, minimal config that reproduces this issue (if it's still an issue on the latest revisions)?

@lindapaiste
Copy link
Contributor

I chatted with @msutkowski yesterday about a filterEndpoints callback function and I thought that was a good idea. Is that something that we want instead of my PR with the excludeEndpoint option, or in addition to it? I got the impression from Matt that we didn't want to go overboard with options.

@Lokaltog
Copy link
Contributor

I'm thinking that it's not necessary with both options as a filterEndpoints callback allows for the same filtering as excludeEndpoints, only with inverted checks. A callback is also more flexible so I'm voting for a single option for filtering.

@grumpyTofu
Copy link
Contributor

grumpyTofu commented Nov 11, 2021

@Lokaltog Unfortunately, I don't have the repo that I am working with on github. However, the config is posted in the comments above. I am also just running it with a yarn script that runs the cli.js file using node and passes the config file as the argument. I wouldn't worry too much about it. I did switch to esr and everything is fine. We could just recommend that in the docs.

@phryneas
Copy link
Member Author

@grumpyTofu could you please test the version I just pushed? I think your import problem should be solved with that.

@grumpyTofu
Copy link
Contributor

@phryneas Was this related to something in my PR? Or are you talking about the broken import Issue #1753? It looks like the broken import issue was opened by @ChuckJonas. However, I can help with the testing efforts.

I can try to replicate the original issue so that I can test if that is the case, but it might be easier if @ChuckJonas tests this to see if it solves the original issue.

@ChuckJonas
Copy link

@phryneas I was just running the code from the example in the docs when I got the broken import issue.

@phryneas
Copy link
Member Author

@grumpyTofu sorry, "export" 😅
I was talking about this one:

$ node ./node_modules/@rtk-query/codegen-openapi/lib/bin/cli.js openapi.dev.config.ts
(node:33479) UnhandledPromiseRejectionWarning: /Users/austinfelix/apps/finsense-ui/openapi.dev.config.ts:10
export default config;
^^^^^^

SyntaxError: Unexpected token 'export'
    at wrapSafe (internal/modules/cjs/loader.js:1001:16)

@lindapaiste
Copy link
Contributor

Can we got PR #1738 merged in? This PR is needed in order to support queries with method POST (which I am personally very excited about!).

We are now allowing the user to specify whether an endpoint is a query or a mutation rather than inferring based on the method. Therefore we can no longer assume that all queries will have method GET.

@lindapaiste
Copy link
Contributor

lindapaiste commented Nov 23, 2021

I am also just running it with a yarn script that runs the cli.js file using node and passes the config file as the argument.

I have encountered the same sort of issue as @grumpyTofu. I would consider it "user error" because I'm pretty sure that it's caused by trying to run the script in an environment that doesn't have access to TS.

The error is not from inside the rtk-query script. It never gets far enough to execute the codegen. The error is that node cannot open a .ts config file. If I switch the config to a .js file with module.exports = syntax then it works. There is no problem executing the codegen. If it's not the package itself that's causing the problem then I don't think that the package can fix it?


Detailed explanation of errors

My setup: calling npm run-script generate -> calls node generate.js -> executes the following file:

const {execSync} = require('child_process');

const DIR = "./src/API";

// download and merge all openapi.json files
execSync(`npx openapi-merge-cli --config ${DIR}/openapi-merge.json`);
// create TS types and RTK query API
execSync(`npx @rtk-query/codegen-openapi ${DIR}/codegen-config.ts`);

I have ts-node installed globally but I don't think execSync can use it. So I get the error:

Encountered a TypeScript configfile, but neither esbuild-runner nor ts-node are installed.
node:child_process:903
    throw err;
    ^

Error: Command failed: npx @rtk-query/codegen-openapi ./src/API/codegen-config.ts
Encountered a TypeScript configfile, but neither esbuild-runner nor ts-node are installed.

So I changed the .ts to a .js and removed the ConfigFile type. Then I get the error regarding export syntax:

Error: Command failed: npx @rtk-query/codegen-openapi ./src/API/codegen-config.js
C:\Users\Linda Paiste\PhpstormProjects\monaco-stable\src\API\codegen-config.js:18
export default config;
^^^^^^

SyntaxError: Unexpected token 'export'

So then I changed my config file to use module.exports = config; and that worked fine. Which confirms that the codegen itself can run in the node execSync environment.


Avoiding Errors

The problem does not happen when I call the commands directly from the script. Instead using the script to execute a file that calls the commands, I can cut out the middle step and put this in my package.json:

  "scripts": {
    "generate": "npx openapi-merge-cli --config ./src/API/openapi-merge.json && npx @rtk-query/codegen-openapi ./src/API/codegen-config.ts",
  },

@grumpyTofu
Copy link
Contributor

@lindapaiste Yes, there is definitely an issue with the script implementation. It's not really related to the use of Javascript vs Typescript but more of an ES module thing. There are several fixes such as changing to a .mjs or .mts file extension which makes the script into an ES module OR change the export to module.exports = config; as you have pointed out. Perhaps the best solution though was to just use esbuild-runner as recommended by @phryneas and then you can write your package.json script as you normally would and it will solve this issue for you. Based on his recent commits, this is what was internally implemented so the cli will perform this behind the scenes. Personally, I like that method and have been using that instead for more than just this use case. Thanks to @phryneas for teaching me something new ;)

@phryneas to follow up on testing, everything looks great and the example config file from the docs should work fine now!

@phryneas phryneas marked this pull request as ready for review November 27, 2021 18:31
@phryneas phryneas merged commit 4163a76 into master Nov 27, 2021
@phryneas phryneas deleted the rtk-query-codegen-openapi branch November 27, 2021 18:34
@phryneas
Copy link
Member Author

This is now released as @rtk-query/[email protected]. If no bug reports come in anytime soon, it will be the final 1.0.0

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.