-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
TypeError: partialRoute.split is not a function #5481
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
Comments
Hi @Michsior14 and thanks for reporting. That looks like a bug introduced in #5450. Apologies for that. In your reproduction steps, you said that starting the app is enough. Can you confirm that? Or does it crash at an incoming request? Thanks for your help! |
@Lms24 Ups, sorry! I've updated the description, obviously it crashes on the request. |
Thanks for clarifying @Michsior14! I poked a bit at the express source code but couldn't yet find out how the To help us debug this, could you tell us about your Express app? I'd be interested in your Sentry config as well as in a route that fails. Doesn't have to be the whole app but enough to reproduce this. Thanks! |
@Lms24 I found out what causes the issue. According to express docs it's allowed to define the paths as regex and one of my apps is using them a lot for different purposes. The code bellow with the latest SDK is a problem: router.get(/\/file/, (_: express.Request, res: express.Response, next: express.NextFunction) => {
next();
}); |
Awesome, thanks @Michsior14! That makes sense. Now that we know how to reproduce it, we'll try to fix this asap. |
make our URL parameterization for Express (introduced in #5450) compatible with RegEx-defined routes. Previously, as reported in #5481, our parameterization logic would cause a crash because instead of a string, the matched route would be of type `RegExp`. This PR adjusts our logic so that we detect if we get a matched string our regex. In the latter case, we also append a `'/'` to the reconstructed partial route name so that the regex is closed.
Hi @Lms24 just for you to know, arrays are also valids path parameters |
Ah I see, Express is really not making this easy 😅 If you have the time, please open a separate issue. We'll release the fix for this one today and I'll take a look at the arrays tomorrow. Thanks @albanv |
Is there an existing issue for this?
How do you use Sentry?
Sentry Saas (sentry.io)
Which package are you using?
@sentry/tracing
SDK Version
7.8.0
Framework Version
express 4.18.1, node 16.15.0
Link to Sentry event
No response
Steps to Reproduce
Expected Result
Requests to the server are processes properly without any errors.
Actual Result
Each request fails with the following error (probably related to #5450):
The text was updated successfully, but these errors were encountered: