Skip to content

Conversation

@LukeHagar
Copy link

@LukeHagar LukeHagar commented Jul 18, 2024

What/Why/How?

Fixes #1634

Reference

Testing

Screenshots (optional)

Check yourself

  • Code changed? - Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests
  • New package installed? - Tested in different environments (browser/node)

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@LukeHagar LukeHagar requested a review from a team as a code owner July 18, 2024 07:41
@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2024

🦋 Changeset detected

Latest commit: aa943dc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@redocly/cli Patch
@redocly/openapi-core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lornajane
Copy link
Contributor

Thanks for the pull request @LukeHagar , we'll take a look.

@tatomyr
Copy link
Collaborator

tatomyr commented Jul 22, 2024

@LukeHagar do you mind adding a simple e2e test for the case?
(The corresponding folder is __tests__/split).

@LukeHagar
Copy link
Author

I'd be happy too, I realize this solution would also collide with any API endpoints that are actually /root so I'll solve for that as well.

@tatomyr tatomyr marked this pull request as draft August 22, 2024 07:46
@LukeHagar LukeHagar marked this pull request as ready for review October 3, 2024 02:03
@LukeHagar
Copy link
Author

@lornajane @tatomyr I think this is right. I couldn't get any of the tests to run locally due to a warning about an outdated dep messing with the snapshot output

@tatomyr
Copy link
Collaborator

tatomyr commented Oct 3, 2024

I couldn't get any of the tests to run locally due to a warning about an outdated dep messing with the snapshot output

It's most likely because you're using Node version 21 or higher. If you try version 20 or earlier, it should work locally.

@LukeHagar
Copy link
Author

Yup I'm on v22.8.0

@LukeHagar LukeHagar requested a review from a team as a code owner October 7, 2024 13:14

export function pathToFilename(path: string, pathSeparator: string) {
if (path === '/') {
return '~root';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that it will create a file named ~root for the root path?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the best solution here would be simply returning the pathSeparator.
@LukeHagar, let me know if are still interested in the PR, or I'll redo this myself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapped it over, but I do think there could be a better solution here, as a root level endpoint is somewhat common.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be right, but I haven't come up with a better solution yet. Let's start small and will see how it goes.

@tatomyr
Copy link
Collaborator

tatomyr commented Oct 25, 2025

Could you rebase your branch as I believe smoke tests are failing due to the outdated code?

@LukeHagar
Copy link
Author

I'll rebase when I get on this morning

@tatomyr
Copy link
Collaborator

tatomyr commented Oct 29, 2025

@LukeHagar it looks like it's too much to rebase. As for me, it'd be simpler to reapply your changes from scratch.

@tatomyr
Copy link
Collaborator

tatomyr commented Nov 4, 2025

Any updates @LukeHagar 🙂?

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.

root level paths / are not split correctly

7 participants