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

Bug: incorrect import path generation #132

Closed
Boorj opened this issue May 22, 2021 · 25 comments
Closed

Bug: incorrect import path generation #132

Boorj opened this issue May 22, 2021 · 25 comments

Comments

@Boorj
Copy link
Contributor

Boorj commented May 22, 2021

In writers/SingleMode.ts there's a part producing incorrect relative schemas path
Code here https://github.com/anymaniax/orval/blob/master/src/core/writers/singleMode.ts#L43

export const writeSingleMode = (...) {

// (1)
 const { path, dirname } = getFileInfo(
     join(workspace, targetedPath),
    camel(info.title),
  );

// (2)
const schemasPath = relative(dirname || '', output.schemas); // ←—— This line

What are the steps to reproduce this issue?

  1. setup output with relative parts:
mode    : 'single',
target  : `../../openapi/orval-build/endpoints/openapi.ts`,
schemas : `../../openapi/orval-build/model`,
  1. run build

What happens?

endpoints/openapi.ts output file will get incorrect import path:

from '..\..\..\..\..\openapi\orval-build\model'

What were you expecting to happen?

but expected to get from '../model';

Any other comments?

So we are getting 2 problems:

  1. inverted slashes (OS separators could be straight or back slashes, but imports must not be backslash).
  2. incorrect relative part.

The solution.

  1. Slashes direction could be easily solved by adding .replace(/\\/g, '/');
  2. When using relative(dirname || '', output.schemas);, dirname is already absolute path, processed with path.join ((1)). But output.schemas is still dirty input from config file (2). So the result is incorrect.
    Fix could look this way:
relative(dirname, join(workspace, output.schemas)).replace(/\\/g, '/');

What versions are you using?

Operating System: Win10
Node 14

@Boorj
Copy link
Contributor Author

Boorj commented May 22, 2021

Btw, the same issue with slashes happens in project with relative paths in several places.
For example here: https://github.com/anymaniax/orval/blob/master/src/core/generators/mutator.ts#L13

@anymaniax
Copy link
Owner

Should be fixed with the 5.3.2. Thanks for the issue 🎉

@Boorj
Copy link
Contributor Author

Boorj commented May 22, 2021

@anymaniax
btw, i will not create additional issue, but this line produces incorrect code if definition is undefined

https://github.com/anymaniax/orval/blob/master/src/core/generators/react-query.ts#L82

// fails
<Data extends unknown ? ${  response.definition  } : Data>

// better
const typing = response.definition 
    ? "<Data extends unknown ? " + response.definition + " : Data>" 
    : '';

@anymaniax
Copy link
Owner

Can you do a pull request to fix it please?

@anymaniax
Copy link
Owner

And if response.definition is falsy the value should be Data to have the generic type

@anymaniax
Copy link
Owner

If you don't have time I will do it tomorrow

@Boorj
Copy link
Contributor Author

Boorj commented May 23, 2021

Sure, PR is on the run) Sorry, timezones lag )
Probably in next versions you can make some functions to do those name extractions consistently.

@Boorj
Copy link
Contributor Author

Boorj commented May 23, 2021

Done. Also i feel like project heavily needs some refactoring to keep DRY-principle and consistency.
There's an utility here : https://github.com/anymaniax/orval/blob/master/src/utils/path.ts#L10 which provides safe relative path. I think that could be used consistently in all places. Also, all imports should be unified to correct slash, and it's missing in code, as i see (haven't check, how dependent utilities from import work).

@Boorj
Copy link
Contributor Author

Boorj commented May 23, 2021

btw,
image
i'm re-writing some code at my side.

Looks like optional type of input and output brought tons of redundant code (typechecks, etc).
Isn't it easier to normalize type on start?

@anymaniax
Copy link
Owner

Be careful, it seems that you are not up to date with master

@anymaniax
Copy link
Owner

But otherwise, it's a good idea

@anymaniax
Copy link
Owner

For the relativeSafe util, it's used everywhere normally in the v5

@anymaniax
Copy link
Owner

And please, if you do some refactoring try to do small pull request. Like that we can easily discuss it.

@Boorj
Copy link
Contributor Author

Boorj commented May 23, 2021

Sure (about tiny commits). And indeed, version was obsolete, sorry

@Boorj
Copy link
Contributor Author

Boorj commented May 23, 2021

@anymaniax Also, Victor, some notes, non related

  1. Library generates msw mocks based on faker, and then you can use them in browser as service worker with interceptor. Also node js could use the same interceptor. Actually, i haven't seen libraries for nodejs+openapi that provide customizable responses. Only based on type. Orval has at least regexp generator for customization. And, actually, that could be quite enough.
    How do you find next idea: generate mocks not as import {rest} from 'msw';, but to tweak output for use as configuration of express/koa/hapi server? You already have a parser, express uses almost the same signature.

  2. Why don't you use templating? this code looks very neat.

@anymaniax
Copy link
Owner

anymaniax commented May 23, 2021

@Boorj I already thought about it and it could be a really good feature. Maybe open a new issue first this one.

For point 2 at the beginning, this project was inspired by restful-react and he wasn't using templates. I also already thought about it but it's a big refactor. I maintain this in my free time and don't have a lot of time for this one. I always thought that there were more important changes to do than that one.

@anymaniax
Copy link
Owner

@Boorj can we close this one?

@anymaniax anymaniax reopened this May 23, 2021
@Boorj
Copy link
Contributor Author

Boorj commented May 23, 2021

@anymaniax yeah, sure. relativeSafe with 'upath' does the job. Thank you!

@Boorj Boorj closed this as completed May 23, 2021
@Boorj
Copy link
Contributor Author

Boorj commented May 23, 2021

Opening again. Encountered error:
https://github.com/anymaniax/orval/blob/master/src/core/generators/api.ts#L17

image

Getting specKey with forward slashes, and specs object has key with filename, containing backslashes.

like

specKey === 'c:/path/foo.json'
specs === { 'c:\path\foo.json\': ...}

@Vylda
Copy link

Vylda commented Jan 12, 2023

well, v. 6.11.0 has the same problem

@anymaniax
Copy link
Owner

Hello @Vylda, I am currently on it. it's the same as this one #725

@anymaniax
Copy link
Owner

Hello @Vylda, can you try if 6.11.1-alpha.1 is working for you

@Vylda
Copy link

Vylda commented Jan 16, 2023

Hello @anymaniax ,
I cant run orval correctly:
`npx orval --config orval.config.ts

🍻 Start orval v6.11.1-alpha.1 - A swagger client generator for typescript
node:internal/process/promises:279
triggerUncaughtException(err, true /* fromPromise */);
^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "failed to load from C:/www/studentagency/orval.config.ts => TypeError: n is not a function".] {
code: 'ERR_UNHANDLED_REJECTION'
}`

@anymaniax
Copy link
Owner

check here #725 did an other alpha version to fix that

@Vylda
Copy link

Vylda commented Jan 17, 2023

6.11.1-alpha.2 looks fine, thank you.

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 a pull request may close this issue.

3 participants