Open XanderEndre opened 1 year ago
@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?
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.
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?
Please drop this PR in favor of https://github.com/kogosoftwarellc/open-api/pull/877 and please merge it. Its a huge pain for us and easily fixed.
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