try to resolve #244

[YanLobat]

The problem was in impossibility to reuse router instance with multiple prefixes properly, because layer instance inside router was overwritten with new routes.

And it is real situation when previous routes won't be accessible anymore. That's why we need to create layer instance in every iteration.

At least, this fix resolves issue in #244 but I would to discover bad cases for this fix if they exist.

[YanLobat]

@jbielick

[jbielick]

It looks like there's a test failure that might be related. Know what might be going on there?

[YanLobat]

Sorry for delay
@jbielick I investigated test failures and finally solved an issue!
The reason of failure is absence of param middleware in Layer's stack, because I created new one now according to solution of issue #244
So now I called param middleware injection manually, not the best option I suppose but it works

[YanLobat]

@alexmingoia @jbielick ping

[YanLobat]

@jbielick 😞 I think this bug is really worth to resolve

[jbielick]

Thanks for this work, @YanLobat . I'm hesitant to merge it because I did not perceive a lot of confidence in the changes being made. The tests were broken until it was realized param handlers weren't transferred to the new layer and I wonder what else might be missing. Additionally, this PR includes no tests to assert the solved issue—it does not protect against regressions and I would say the impact of creating a new layer and adding it to the stack doesn't seem completely scoped out. For a change that has such a broad reach of impact to the library I was hoping to see more information and explanation in the PR description, as well as tests and information on how users might need to change their code (or not change their code) in their applications to solve the issue. Any thoughts?

[YanLobat]

Thanks for reply @jbielick ! That PR will cause more memory consumption while shared routers are used because of creating new instances of router Layers. As well such thing as param should be reset if it presents. About tests, you are totally right. I will perform one. Example from #244 is worth for it?
And about impact, generally the pipeline how routers are built is still the same, I just investigated how routes is working generally and afterwards shared router usage, found a bug about layer immutability which you mentioned in issue #244. And resolved it. That's why I believe nothing is missing.

[wong2]

Any progress?

[YanLobat]

@wong2 I am so sorry for delay!! I'll add tests this week and will wait until @jbielick resolution

[wong2]

@YanLobat thanks! I believe lots of people are waiting for this!

[YanLobat]

@jbielick Hi, I updated tests and add one from #244
In latest comment I described how the PR works, hope all is fine.

[YanLobat]

Hi @jbielick. Any updates?
Or as far as I understand you are working on v8.x currently? If you want I can give a hand.

[jbielick]

Can you remove package-lock.json from this PR?

[YanLobat]

There are no merge conflicts anymore! Seems like #433 worked out

[YanLobat]

I guess we can merge it now with no blockers. What're your thoughts @jbielick?

[jbielick]

@YanLobat can you request contributor access from @alexmingoia ? At that point you could merge and handle any issues / releases for the that work (which would probably warrant a new version of the router since it could have some sneaky implications for anyone who has tried a workaround or patched it differently).

[AlbertMarashi]

Why hasn't this been merged yet?

[YanLobat]

@jbielick Yes I can but it's pretty obvious for me that this one can be merged. Still don't get the reason why it's not merged yet.

@alexmingoia Can I have contributor access?

@DominusVilicus Have no idea.

[jbielick]

@YanLobat I don’t wish to continue triaging issues and maintaining this lib, so I’d like to recuse myself of updating the change log, publishing a new version (major, since this has implications for anyone who worked around this issue), and being the only one who could help resolve any issues that arise from this release.

I think you can obtain contributor abilities if you would like to take that on and release this fix—that way if there are any issues you can also be able to address and fix those (merge PRs) without needing my help.

If I had the ability to give you contributor status I would. I don’t feel comfortable, personally, merging this and answering any questions in issues—there are many related to this and I believe that will continue to be the case (not all of them are solved by this PR—see issues tagged with “nested routes”).

/cc @alexmingoia

[jbielick]

One thing I can say for certain is that the tests don’t cover all of the use cases for this lib and passing tests don’t provide the same confidence to me that they might to others. This became apparent when attempting to write 8.x

[YanLobat]

@jbielick Thanks for the detailed feedback!

[edevil]

@YanLobat Why is package-lock.json touched in this PR? Are updated deps needed for this functionality?