smapiot / piral

🚀 Framework for next generation web apps using micro frontends. ⭐️ Star to support our work!
https://piral.io
MIT License
1.71k stars 127 forks source link

Setting to configure React Router version #713

Closed jared-hexagon closed 1 month ago

jared-hexagon commented 2 months ago

New Feature Proposal

Description

Instead of detecting the React Router version by searching inside node_modules, add a setting in createInstance or an env var or a package.json setting or whatever to either disable the in-built React Router usage or force your own version.

By the way - in createInstance when providing your own components like RouteSwitch etc. I would expect Piral not to import any React Router components since I am providing my own.

Background

In my project I have a dependency on an intermediate package that then depends on React Router so my app never depends on React Router itself, so Piral can never determine it and always falls back to 5 (even though the intermediate package is on 6).

Discussion

Pro - Devs bringing their own routers is good. Piral doesn't have to have a strict dependency on React Router especially because we can override all of the components.

Bad - more settings means more maintenance/code/tests for that setting to work.

FlorianRappl commented 2 months ago

This is already possible - just add an explicit dependency to react-router V6.

FlorianRappl commented 2 months ago

Did this solve your problem? Any other suggestions?

FlorianRappl commented 2 months ago

Quick ping @jared-hexagon - was your issue resolved by the comment above? Anything unclear?

I'd close the issue as this is already possible if there are no objections.

jared-hexagon commented 2 months ago

This is already possible - just add an explicit dependency to react-router V6.

This is the whole reason why I created this issue - I don't want to.

I looked at how it detects react-router and it does so by looking into your node_modules and inspecting react-router's package.json version.

This works fine in most cases but in my project's case I use a different package to handle routing which itself depends on react-router. My app should hopefully never depend on react-router.

Although your way of detecting react-router works in most cases, it is a bit fragile and is not compatible with some other types of React apps. I would like any way to override the react-router version.

FlorianRappl commented 2 months ago

But how is setting the version in a config file different from seeing the version in the package.json?

I don't get the refusal and why it should be more fragile. After all, we need the package and has to be discoverable by the bundler. So it has to be at a known location in the node_modules.

Maybe you can make a proposal how it should work technically and we discuss that?

jared-hexagon commented 2 months ago

But how is setting the version in a config file different from seeing the version in the package.json?

I don't depend on react-router in my project. So it cannot see the version in the package.json. So it falls back to react-router 5 and the imports fail.

Maybe you can make a proposal how it should work technically and we discuss that?

Add env var PIRAL_REACT_ROUTER_VERSION=6

Even better would be the option of never importing React Router ever so the user can replace all React Router components with their own copy (which might be an even better version of React Router like 7+ or a completely different router).

I don't get the refusal and why it should be more fragile

Any time you read a package.json from a node_module to do detection that is fragile imo. Not many packages ever do this because it's not reliable. My use case is an example.

FlorianRappl commented 2 months ago

Sorry but that does not make much sense to me.

  1. With the env you state an explicit dependency, but the package.json is the location for specifying this.

  2. Your approach allows naming packages that might not be installed.

  3. Whether you see it directly or not - you are using react-router. Piral is a framework, as such it can make this decision. A library should not make the decision (otherwise it would be a framework).

I don't see how the current approach is fragile - or more fragile than what you've shown. Let's say we go with your proposal: how can a bundler / or the Piral CLI resolve it? You set the version, but where is the package? If your current scenario is the case then the package might be nested inside the library you've mentioned (remember that piral already brings a version of react-router - so conflicting versions will be nested).

And again you can do all of that and more today. The installed packages are used to detect a suitable version of react-router. If you need something else / special you can override all the components that use the router.

What I can imagine is that you set an explicit path (can also be a package name) of a custom router implementation. This, however, would not be the react-router but rather a package that contains the components as used by piral. That way you can actually override it with whatever.

FlorianRappl commented 1 month ago

I close this for now as the router can be changed with the respective installation.

As Piral v2 is coming closer (and this one does not have any dependency to react-router) I'd say the issue has an expiry date anyway.