-
Notifications
You must be signed in to change notification settings - Fork 148
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
1.23.0 release caused breaking change in bundle output option when APIs are not specified #1776
Comments
Hi @imdex-brett-debeer, could you clarify what you mean by API_FILES? Are you using apis:
orders@v1:
root: orders/openapi.yaml
output: dist/orders.json
accounts@v1:
root: accounts/openapi.yaml
output: dist/accounts.json
|
See [Redocly/redocly-cli #1776](Redocly/redocly-cli#1776)
Hi @tatomyr, Apologies API_FILES was copied from #1715 by mistake. I've created redocly-bundle-repro as a minimal reproduction. |
Thanks! I’d still recommend defining the output paths explicitly in your config file though. The issue with the previous behaviour is that some might refer to the same API description file multiple times in the apis section with different decorators so they'll be stored under the same name, making it unclear which exact bundle gets stored in the output. |
Hi @tatomyr, Setting the output path in the config file is not equivalent to the behaviour previously provided, unfortunately. |
Yes, that was a deliberate decision to avoid any ambiguity about which file represents each of the In your example, you can achieve the same behaviour as before just by specifying your original apis (e.g. using the glob pattern):
Please let me know if that works for you. |
…li#1776 (comment)) - added output properties to the config file. - updated the command to the recommended one. - added the museum@json API that uses the same root file as the museum API
I've updated my reproduction repository to demonstrate. The test below should also demonstrate what occurs if added to packages/cli/src/tests/commands/bundle.test.ts it('should store bundled API descriptions in the directory specified in argv IF multiple positional api names provided AND --output specified', async () => {
const apis = {
foo: {
root: 'foo.yaml',
output: 'output/foo.yaml',
},
fooJson: {
root: 'foo.yaml',
output: 'output/foo.json',
},
bar: {
root: 'bar.yaml',
output: 'output/bar.json',
},
};
const config = {
apis,
styleguide: {
skipPreprocessors: jest.fn(),
skipDecorators: jest.fn(),
},
} as unknown as Config;
// @ts-ignore
getFallbackApisOrExit = jest
.fn()
.mockResolvedValueOnce(
Object.entries(apis).map(([alias, { root, ...api }]) => ({ ...api, path: root, alias }))
);
(getTotals as jest.Mock).mockReturnValue({
errors: 0,
warnings: 0,
ignored: 0,
});
await handleBundle({
argv: { apis: ['foo', 'fooJson', 'bar'], output: 'dist' }, // cli options
version: 'test',
config,
});
expect(saveBundle).toBeCalledTimes(3);
expect(saveBundle).toHaveBeenNthCalledWith(1, 'dist/foo.yaml', expect.any(String));
expect(saveBundle).toHaveBeenNthCalledWith(2, 'dist/foo.json', expect.any(String));
expect(saveBundle).toHaveBeenNthCalledWith(3, 'dist/bar.yaml', expect.any(String));
}); |
Yes, and that's one of the reasons the The case in your latest example wouldn't work with a version prior to v1.23.0 (unless I'm missing something) because:
And I still don't get why using the |
Describe the bug
Release 1.23.0 introduced #1715. Fix #1717 resolved the issue when API names are provided as arguments but did not correct the problem when no API names are provided.
To Reproduce
We have the following make command to bundle some spec files:
As of 1.23.0, it outputs a single file named generated.yaml in the ./ folder:
Expected behavior
Before, it would output one file per spec file to the ./generated/ folder.
OpenAPI description
OAS 3.0.0
Redocly Version(s)
I've tried the following versions but expect it applies to all versions since 1.23.0
Node.js
Version(s)v20.10.0
OS, environment
Ubuntu 22.04.5 LTS in WSL on Windows 11
Additional context
Changing redocly-cli/packages/cli/src/commands/bundle.ts line 78 to the below might solve the problem.
The text was updated successfully, but these errors were encountered: