kruschid / typesafe-routes

Spices up your favorite routing library by adding type safety to plain string-based route definitions.
https://kruschid.github.io/typesafe-routes/
MIT License
103 stars 8 forks source link

`routeFn` assumes `this` is defined #3

Closed jeysal closed 3 years ago

jeysal commented 3 years ago

Hi, first of all thanks a lot for creating this module. I'm introducing it in a medium-sized frontend project after noticing that it does pretty much exactly what I was starting to write, only with a slightly different API. So you may see at least a small usage spike soon ;)

I've hit two issues trying to adopt the library, one of which I'm reporting here, the other one I'll create a PR for.

Repro

Add the dependency, then node -p 'const {route} = require("typesafe-routes"); route("/root", {}, {})({}).$'

Expected

Prints /root

Actual

/tmp/asdf/node_modules/typesafe-routes/build/index.js:88
            var queryParams = __assign(__assign({}, _this.previousQueryParams), stringifyParams(parsedRoute.queryParamParsers, rawParams));
                                                          ^
TypeError: Cannot read property 'previousQueryParams' of undefined
    at Object.get (/tmp/asdf/node_modules/typesafe-routes/build/index.js:88:59)
    at [eval]:1:71    at Script.runInThisContext (vm.js:132:18)
    at Object.runInThisContext (vm.js:309:38)
    at internal/process/execution.js:77:19
    at [eval]-wrapper:6:22
    at evalScript (internal/process/execution.js:76:60)
    at internal/main/eval_string.js:23:3

Note

In the given repro, the this of routeFn will be undefined (which the function fails to deal with), unlike when doing node -p 'require("typesafe-routes").route("/root", {}, {})({}).$', which works as expected.

kruschid commented 3 years ago

Hi, first of all thanks a lot for creating this module. I'm introducing it in a medium-sized frontend project after noticing that it does pretty much exactly what I was starting to write, only with a slightly different API. So you may see at least a small usage spike soon ;)

Hi @jeysal, thank you for your feedback. That sounds pretty good. Yes the current API is a tradeoff between type safety and a reasonable dev experience. I've tried different things but in the end I stuck with the current implementation. If you think you can improve the api, or if you discover an important feature along the way, I would appreciate you to share it with me. :+1: .

TypeError: Cannot read property 'previousQueryParams' of undefined at Object.get (/tmp/asdf/node_modules/typesafe-routes/build/index.js:88:59) ...

Well, I guess strict mode saved us from potentially worse bugs here (positive thinking :-)).

I've just published 7.0.1 on npm.

Let me know if you discover more issues related to this lib. I'm always happy to help.

jeysal commented 3 years ago

Thanks, 7.0.1 works like a charm! I'm still getting a weird typing issue with the imported useRouteParams that does not happen if I copy it over into our code, will have to investigate that.

Yeah regarding the api, I didn't get far building my own thing until I noticed it wasn't a trivial 20 lines thing to build and started searching for pre-made things, so I don't really have an alternative to offer and don't know whether I could come up with something better. My guess is it's probably about as good as it gets. I really like that unlike many solutions out there it is just used alongside a router - many others are something like "replace react router with a type-safe version". Special name $ is slightly weird at first but that's what you pay for directly chaining children via .myChildRoute as opposed to .children.myChildRoute. Only alternative would be hiding it behind a symbol or something and making the API buildUrl(myRoute({})). But that's more code just to avoid a magic $ which doesn't actually hurt anyone, just takes a short while getting used to it.

I did encounter one thing while migrating our app over that I couldn't quite express in a nice way, perhaps I'll open a discussion issue about that.