search

slovnicki / beamer

A routing package built on top of Router and Navigator's pages API, supporting arbitrary nested navigation, guards and more.
MIT License
591 stars 129 forks source link

Fix regexp path patern's not all checked #670

Closed stan-at-work closed 3 months ago

stan-at-work commented 4 months ago

Summary

Issue: Regular expression (regexp) path patterns are not being fully checked during the path pattern matching process.

Detailed Description

I encountered this issue while working with path pattern matching. The core problem lies in the handling of regular expression (regexp) path patterns within the codebase. Specifically, the current implementation fails to iterate over all provided path patterns, stopping the process prematurely when it encounters a failure. This limitation significantly restricts the utility and flexibility of using regular expressions for path matching.

Solution

The proposed fix ensures that all path patterns are checked by modifying the iteration logic. Instead of returning upon the first failure, the code now continues to iterate through the entire list of path patterns, thereby improving the robustness and reliability of the regexp path pattern matching.

By applying this fix, the system will be able to handle a more extensive range of path patterns, thus enhancing its functionality and accommodating more complex use cases.

slovnicki commented 4 months ago

Oh wow, what a find! I'm surprised how this was missed and not noticed sooner. (probably not a lot of people using regex path patterns, me included)

Thank you very much for the PR! 💙

Before merging, I would like to take more time for these 2 things;

stan-at-work commented 4 months ago

Oh wow, what a find! I'm surprised how this was missed and not noticed sooner. (probably not a lot of people using regex path patterns, me included)

Thank you very much for the PR! 💙

Before merging, I would like to take more time for these 2 things;

  • figure out why we seem to have a duplication of that logic in both beam_guard and utils, and can we alleviate that
  • write at least 1 test for this

Point 1

I think its. Best to put it all in the Utils class.

slovnicki commented 4 months ago

I think its. Best to put it all in the Utils class.

I agree, let's try to refactor this to be only in utils.

Sten435 commented 3 months ago

@slovnicki I created a new PR with some changes and new issues fixed.

https://github.com/slovnicki/beamer/pull/674

Sten435 commented 3 months ago

Closing this, as it has no value anymore