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

express-openapi: fixed windows path names #869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

XanderEndre
Copy link

Windows path names are not being converted correctly. This modified function will take an OpenAPI path, replace all '{param}' style parameters with ':param' style (as used by Express), and also replace all backslashes () with forward slashes (/), making it compatible with all operating systems.

References the issue #868

Windows path names are not being converted correctly. This modified function will take an OpenAPI path, replace all '{param}' style parameters with ':param' style (as used by Express), and also replace all backslashes (\) with forward slashes (/), making it compatible with all operating systems.
@jsdevel
Copy link
Contributor

jsdevel commented May 24, 2023

@ShadyAlexCodes is there a way to add tests for this? i'm assuming we'd need the ability to have these tests run on windows to do so? is there any other way?

@XanderEndre
Copy link
Author

I'm sorry for not getting back to you sooner.

We can undoubtedly add tests for this change. It's worth noting that these tests don't necessarily need to be run on Windows. Since the changes we're making here are at the string transformation level, they are OS-agnostic and can be tested on any system.

However, I need to figure out how to incorporate these new tests into your existing testing framework. Therefore, I'd appreciate any guidance or suggestions you could give me.

@renkei
Copy link

renkei commented Jun 1, 2023

Instead of patching the npm package express-openapi I would suggest to patch the npm package fs-routes. What do you think?

For testing, I've modified node_modules/fs-routes/dist/index.js at line 44:

from

route: '/' + file.replace(options.indexFileRegExp, '')

to

route: '/' + file.replace(options.indexFileRegExp, '').replace(/\\/g, '/')

Then all other related openapi packages like openapi-framework, express-openapi, etc. don't need any patches. Swagger documentation looks also good again. At least for me, it works fine on Windows again, can someone confirm this?

@mystiker
Copy link

mystiker commented Oct 4, 2024

Please drop this PR in favor of #877 and please merge it. Its a huge pain for us and easily fixed.

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.

4 participants