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

Update contraint check to throw with 32 handlers #344

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Update contraint check to throw with 32 handlers #344

merged 1 commit into from
Jan 17, 2024

Conversation

valeneiko
Copy link
Contributor

Comment on lines +50 to +51
if (!isMergedTree && this.handlers.length >= 31) {
throw new Error('find-my-way supports a maximum of 31 route handlers per node when there are constraints, limit reached')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (!isMergedTree && this.handlers.length >= 31) {
throw new Error('find-my-way supports a maximum of 31 route handlers per node when there are constraints, limit reached')
if (!isMergedTree && this.handlers.length >= 33) {
throw new Error('find-my-way supports a maximum of 32 route handlers per node when there are constraints, limit reached')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this the opposite of what we want?

The logic on main throws when we add 33 handlers, but routing breaks with 32.

So what I was trying to achieve with changing to 31 is to throw when we know routing breaks. The reason we need to use 31 here, is because the check is done before we have added current handler to the list.


const findMyWay = FindMyWay()

for (let i = 0; i < 31; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (let i = 0; i < 31; i++) {
for (let i = 0; i < 32; i++) {


const findMyWay = FindMyWay()

for (let i = 0; i < 31; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (let i = 0; i < 31; i++) {
for (let i = 0; i < 33; i++) {

@mcollina
Copy link
Collaborator

@ivan-tymoshenko can you take a look?

Copy link
Collaborator

@ivan-tymoshenko ivan-tymoshenko left a comment

Choose a reason for hiding this comment

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

Indeed it doesn't work for 32 constrainst values, for multiple reasons. One of them is that indexing constraints values from 1. Moving limit ot 31 seems as good idea.

@mcollina mcollina merged commit f3170bd into delvedor:main Jan 17, 2024
12 checks passed
@valeneiko valeneiko deleted the fix-343 branch January 17, 2024 14:35
Josh-Walker-GM referenced this pull request in redwoodjs/redwood May 16, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [find-my-way](https://github.com/delvedor/find-my-way) | [`8.1.0` ->
`8.2.0`](https://renovatebot.com/diffs/npm/find-my-way/8.1.0/8.2.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/find-my-way/8.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/find-my-way/8.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/find-my-way/8.1.0/8.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/find-my-way/8.1.0/8.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>delvedor/find-my-way (find-my-way)</summary>

###
[`v8.2.0`](https://github.com/delvedor/find-my-way/releases/tag/v8.2.0)

[Compare
Source](https://github.com/delvedor/find-my-way/compare/v8.1.0...v8.2.0)

#### What's Changed

- Update contraint check to throw with 32 handlers by
[@&#8203;valeneiko](https://github.com/valeneiko) in
[https://github.com/delvedor/find-my-way/pull/344](https://github.com/delvedor/find-my-way/pull/344)
- feat: add node: prefix for assert by
[@&#8203;SavaCool122](https://github.com/SavaCool122) in
[https://github.com/delvedor/find-my-way/pull/347](https://github.com/delvedor/find-my-way/pull/347)
- add dependabot.yml by [@&#8203;Uzlopak](https://github.com/Uzlopak)
in
[https://github.com/delvedor/find-my-way/pull/350](https://github.com/delvedor/find-my-way/pull/350)
- chore: bump actions/setup-node from 3 to 4 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/delvedor/find-my-way/pull/351](https://github.com/delvedor/find-my-way/pull/351)
- chore: bump actions/checkout from 3 to 4 by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/delvedor/find-my-way/pull/352](https://github.com/delvedor/find-my-way/pull/352)
- Achieve 100% test coverage by
[@&#8203;jean-michelet](https://github.com/jean-michelet) in
[https://github.com/delvedor/find-my-way/pull/349](https://github.com/delvedor/find-my-way/pull/349)
- chore: bump the dependencies-major group with 1 update by
[@&#8203;dependabot](https://github.com/dependabot) in
[https://github.com/delvedor/find-my-way/pull/353](https://github.com/delvedor/find-my-way/pull/353)
- Fix header in README by [@&#8203;selimb](https://github.com/selimb)
in
[https://github.com/delvedor/find-my-way/pull/345](https://github.com/delvedor/find-my-way/pull/345)
- Exclude Node v14 and v16 on macos by
[@&#8203;mcollina](https://github.com/mcollina) in
[https://github.com/delvedor/find-my-way/pull/364](https://github.com/delvedor/find-my-way/pull/364)
- add node v22. Skip old nodes on mac by
[@&#8203;mcollina](https://github.com/mcollina) in
[https://github.com/delvedor/find-my-way/pull/363](https://github.com/delvedor/find-my-way/pull/363)
- Support optional params on root by
[@&#8203;mcollina](https://github.com/mcollina) in
[https://github.com/delvedor/find-my-way/pull/367](https://github.com/delvedor/find-my-way/pull/367)

#### New Contributors

- [@&#8203;valeneiko](https://github.com/valeneiko) made their first
contribution in
[https://github.com/delvedor/find-my-way/pull/344](https://github.com/delvedor/find-my-way/pull/344)
- [@&#8203;SavaCool122](https://github.com/SavaCool122) made their
first contribution in
[https://github.com/delvedor/find-my-way/pull/347](https://github.com/delvedor/find-my-way/pull/347)
- [@&#8203;dependabot](https://github.com/dependabot) made their first
contribution in
[https://github.com/delvedor/find-my-way/pull/351](https://github.com/delvedor/find-my-way/pull/351)
- [@&#8203;jean-michelet](https://github.com/jean-michelet) made their
first contribution in
[https://github.com/delvedor/find-my-way/pull/349](https://github.com/delvedor/find-my-way/pull/349)
- [@&#8203;selimb](https://github.com/selimb) made their first
contribution in
[https://github.com/delvedor/find-my-way/pull/345](https://github.com/delvedor/find-my-way/pull/345)

**Full Changelog**:
delvedor/find-my-way@v8.1.0...v8.2.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/redwoodjs/redwood).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNjMuNSIsInVwZGF0ZWRJblZlciI6IjM3LjM2My41IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

3 participants