search

pillarjs / router

Simple middleware-style router
MIT License
410 stars 103 forks source link

should this library maintain support for isomorphism? #128

Open ctcpip opened 1 month ago

ctcpip commented 1 month ago

In recent discussion, it was mentioned that this library is used by some downstream consumers in the browser, using browserify or similar mechanisms. (There is also some historical discussion on browser support.)

We cannot support browser environments passively -- if this library is expected to work in the browser, then we need to take steps to ensure that it runs in the browser and will continue to run in the browser. This is critical for back-compat but also for forward-compat. This should minimally involve adding some level of CI smoke testing in browsers.

Therefore, this issue aims to answer the question: should this library maintain support for isomorphism?

wesleytodd commented 1 month ago

As the person making that case (in 2015 😬) I can for sure say that the ecosystem has solutions for this type of thing now which it did not have back then. The use case was never popular by any means, but I don't like saying we should explicitly not support it when it for sure works.

Frankly I think the fact that React's ecosystem chose to put routing IN the UI component tree was a design mistake. That said, who am I to argue with something so popular and the other frameworks I have not really worked with beyond toy examples to say much about them. Unless we can work out a really clear recommendation for folks serving UI's from express apps that rules out running any express code in the browser, I think this approach is "technically good".

Now, that would mean we need to setup testing, and that was the real blocker in the past. It is possible (I hope obviously), but no one was there to do the work. And unless we have someone to do that I would hesitate to commit to it. So I would lean toward asking if anyone was interested in helping get browser testing working, and then if we get someone in a reasonable time we say we support it and if not we officially drop support. Unfortunately because of #125, if we went that way, would need to be backed out until we make a formal decision (right? or is there another good option I am not thinking of?).

blakeembrey commented 1 month ago

For this particular package, I don't think we should attempt to make it run in a browser. Any attempt at this point is probably mistaken, and with the latest major release I think it's fair we take that as the opportunity to flag to anyone who breaks that it only runs in node Vx and nowhere else. It seems fair to re-evaluate if there's strong feedback otherwise, but up to the maintainer, if it were me I'd note this is specifically "node http" supported.

ctcpip commented 1 month ago

I mentioned this in the meeting yesterday, but I did take a stab at running it and tests in the browser, and ran into a couple walls.

The status quo today is either:

I am sympathetic to the latter, but as discussed in the meeting yesterday, since v2.0.0 dropped support for node <18, #125 does not need to be backed out because no major rev is needed here.

By saying we support minimum Node 18, for implicit browser support, that means we support minimum V8 10.1, therefore by extension, minimum supported browser versions are approximately:

All of these are WELL past when setPrototypeOf was available (2014 in chrome 34, 2015 in node 0.12)