fenok / react-router-typesafe-routes

Comprehensive and extensible type-safe routes for React Router v6 with first-class support for nested routes and param validation.
MIT License
145 stars 3 forks source link

Add CommonJS support #30

Closed j0nas closed 1 year ago

j0nas commented 1 year ago

I'm using react-router-typesafe-routes in a TypeScript project that compiles with Vite. Vite has no issues with the ESM-only output of this package, but I am unable to run my Playwright tests because they require CommonJS imports.

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/user/Desktop/path/to/project/node_modules/react-router-typesafe-routes/dom/index.js from /Users/user/Desktop/ruter/path/to/project/src/routes.ts not supported.
Instead change the require of index.js in /Users/jonasjensen/Desktop/ruter/bestillingstransport/apps/bt-frontend/src/admin/routing/routes.ts to a dynamic import() which is available in all CommonJS modules.

   at ../src/admin/routing/routes.ts:7

   5 |   OVERVIEW: route('overview'),
   6 |   PROFILE: route('profile'),
>  7 |   TRIPS: route('trips', {}, {LIVE: route('live')}),
     |            ^
   8 |   MORE_ROUTES: route('more_routes', {}, {CLIENTS: route('clients')}),

I unfortunately discovered this only after I'd already rewrite all hardcoded paths in my entire codebase to use react-router-typesafe-routes. 😬 It would be very helpful if the package could support both ESM and CommonJS formats.

It would be great if react-router-typesafe-routes could provide a CommonJS output alongside the ESM output. This would make the package compatible with various testing environments and setups, including those that rely on CommonJS.

(As a workaround, I've tried bundling and transforming my tests using esbuild, but this adds too much extra complexity and I couldn't even get it to work properly..)

Thank you for considering this feature request, and I appreciate your efforts in creating and maintaining this package! I compared all the different "typesafe routes" packages that I could find and this one was by far the best I could find. πŸ™Œ

fenok commented 1 year ago

Thank you! Unfortunately, I don't have experience in creating packages that provide both ESM and CommonJS. It seems pretty easy to mess up.

I'll try to implement something like this, but it will take some time, and I expect additional issues from the fact that the library has multiple entry points.

I also want to roll out https://github.com/fenok/react-router-typesafe-routes/pull/29 first.

If you know a better approach to hybrid packages, please share. The one above feels ugly πŸ˜….

A PR is also welcome.

j0nas commented 1 year ago

Thank you for the swift reply! πŸ™Œ

Have you looked into Microbundle? It's been a while since I used it, but I remember that it handled all the various export formats flawlessly for my package.

I would absolutely not mind contributing, but it will likely be much faster for you to migrate this project to e.g. Microbundle (if you choose to go that route), since you're more familiar with the project than I am. Also, #29 seems like it's big enough to warrant waiting for in either case, do you happen to have an ETA for that? πŸ™‚

fenok commented 1 year ago

I tried to use Microbundle some time ago, but I had some issues and resorted to using Rollup directly. I'll give it a try though, thanks for reminding me about it.

do you happen to have an ETA for that?

Somewhere within a week, if everything goes smoothly.

j0nas commented 1 year ago

Awesome. Let me know how it goes, I definitely wouldn't be opposed to picthing in if need be. :)

fenok commented 1 year ago

In the end, I went with Microbundle. I published CommonJS support under the next tag. It seems to work at the first glance, but I want to test it a bit more.

j0nas commented 1 year ago

That's awesome! Let me know when you're satisfied with your testing, and I'll bump to 1.2.x when it's out, migrate my test suite and let you know how it went.

fenok commented 1 year ago

To (hopefully) prevent breaking changes, I made it so that CJS will only be used if the exports field is supported. As far as I understand, it's enough to resolve the issue with Playwright. Can you please confirm that 1.2.0-dev.4 works for you? If yes, I will release it as 1.2.0 right away.

fenok commented 1 year ago

Wait, I realized that microbundle produces independent bundles, which leads to code duplication. What I actually need is bundles that depend on each other. For instance, zod.js should import stuff from index.js (and zod.cjs should require stuff from index.cjs). Maybe bundling is a bad idea altogether.

j0nas commented 1 year ago

That doesn't sound right, why would you want that? The three bundles should indeed be separate.

I'll get to testing this ASAP and will let you know.

j0nas commented 1 year ago

@fenok 1.2.0-dev.4 works great, fantastic job!

1.1.0: image

1.2.0-dev.4: image

fenok commented 1 year ago

Awesome! It means that 1.2.0-dev.5 should work too, but can you please check it as well? πŸ˜…

If it works, there won't be any significant changes, and it will be released shortly (at this point my only concern is the robustness of build scripts).

That doesn't sound right, why would you want that?

As I said, bundling leads to code duplication. For instance, the /zod entry point should only contain a simple wrapper for type(), but, since it imports type, its code gets bundled as well and is essentially loaded twice if /zod is used (we can't use /zod without /dom or /native).

I ditched bundling, and now I simply build the library twice with TS, once for each module type.

j0nas commented 1 year ago

image

dev.5 works equally well πŸ™Œ

Alright, thanks for the recap. I don't have much experience with bundling for package. Very grateful that you took the time and effort to look into this! ❀️

fenok commented 1 year ago

Released in v1.2.0.