IBM / mobx-react-router

Keep your MobX state in sync with react-router
MIT License
439 stars 48 forks source link

Newly created store missing history and location in react-router 6.x #181

Closed kkollman closed 4 months ago

kkollman commented 8 months ago

Hi there!

I am using react-router @6.21 and it seems now mobx RouterStore is missing the History and Location. How should I ensure the history is synced with the router in the new react-router syntax?

gcattan commented 8 months ago

Hi, thanks for reaching out!

react-router v6 is still in the box and we have no stable release for now. However, you can try to pull directly from the git repo:

npm git+https://github.com/IBM/mobx-react-router.git@master

And we can try to figure out how to make it work with your project :)

kkollman commented 8 months ago

Oh, great! Maybe we'll find out I can keep using the official release version.

So, maybe the first thing is what I want to achieve: I want to be able to use location- and history-based methods, similar to those provided by hooks, outside the components. Namely, I want to observe location.pathname and react when they change, in one of the stores. Also, I keep many abstract models in store and want to be able to navigate the app from there. Is that possible outside mobx-react-router?

In the past, I would simply use Mobx Router and synchronise the history via Router provider, but in react-router 6x Router component doesn't accept history. I create a router via createBrowserRouter and add <RouterProvider router={MyRouter} /> in my App.js; the general Store provider is located higher in the tree, like this:

<StoreProvider>
  <IntlProvider>
    <Helmet/>
    <RouterProvider/>
  </IntlProvider>
</StoreProvider>

I tried using that MyRouter as a base for creating a custom Store, but that won't work since Stores are defined higher in the tree (and many Route elements already use them).

gcattan commented 8 months ago

The master branch of this git repo is on react-router v6. That "should" work out of the box.

I would recommend installing the development version using npm, like this:

npm git+https://github.com/IBM/mobx-react-router.git@master

Then, let's if you still have the type error.

gcattan commented 8 months ago

Additionally, is your code available on GitHub? Maybe I can try also on my end to integrate with your code.

kkollman commented 8 months ago

@gcattan unfortunately, it's a private codebase and I cannot share it. But I'll try the branch you mentioned now!

kkollman commented 8 months ago

@gcattan I'm sorry, but it seems simply running the command you provided throws an error.

EDIT: Okay, sorry, I have it - it was yarn add IBM/mobx-react-router#master :)

kkollman commented 8 months ago

@gcattan Unfortunately, installing the package makes the app crash with an error: Failed to resolve entry for package "@ibm/mobx-react-router". The package may have incorrect main/module/exports specified in its package.json. For what it's worth - I am using Vite for bundling.

gcattan commented 8 months ago

Oops!

Let try:

npm install git+https://github.com/IBM/mobx-react-router.git#master

or

yarn add git+https://github.com/IBM/mobx-react-router.git#master

kkollman commented 8 months ago

@gcattan Unfortunately, the same problem still persists:

[plugin:vite:import-analysis] Failed to resolve entry for package "@ibm/mobx-react-router". The package may have incorrect main/module/exports specified in its package.json.

Weirdly enough, WebStorm IDE correctly identifies the element to be imported, but Vite.js cannot. Worth noting, installation went fine.

gcattan commented 8 months ago

Arf. I do not see this issue with webpack. I will try to setup a stackblitz with vite.

Can you try once again to pull from the git repo, see if adding the module property to the package.json helped?

kkollman commented 7 months ago

@gcattan thank you for adding the module property! I re-installed node modules and pulled the newest version from this repo, but unfortunately the problem still exists.

kkollman commented 7 months ago

@gcattan is it possible to observe History and Location outside of mobx router?

gcattan commented 7 months ago

@kkollman yes, sorry for the long delay, I still did not have time to setup an stackblitz with Vite.js.

You could try the useLocation hook.

Or something like this for example:

const onLocationChange = (history) => {
    const { location } = history;
    // Do something awesome with location
  };

history.listen(onLocationChange);
kkollman commented 7 months ago

No problem about the delay! Thank you for your suggestion, but it has an important caveat - I intend to use History and Location outside of the component. I have a collection of models kept in my mobx store, and some of them should be able to navigate to some route or change the search queries. These things are easy to achieve with hooks, unfortunately, those aren't available for non-component elements.

gcattan commented 7 months ago

I understand. In this case, I would recommend to go with history.listen.

Unfortunately, I was not able to reproduce your bug this morning on my local machine. I am using vite 5@5.0.12 and node v18.19.0 and yarn 1.22.21

kkollman commented 7 months ago

Unfortunately, I was not able to reproduce your bug this morning on my local machine. That's a bummer. I am using the same versions of Vite, node and yarn, but the bug still comes up when I start the app. Does it work out of the box for you?

In that case, I'll try to work around it, referring directly to the history library then. Thank you for your assistance anyway!

gcattan commented 7 months ago

No worries. I will leave this issue open in any case. I hope we can understand what is going on here at some point. Thank you for reporting this :)

kkollman commented 7 months ago

@gcattan Sorry I am calling you once again, but I have been using mobx-react-router for years and it would be a shame to drop it because of the React Router update :) So, I tried to set up a completely new project to test if this library conflicted with any of my other libraries. I ended up with the same error. Maybe that will shed some light on the reason it fails. What I did was:

% yarn create vite ReactMobxRouterTest --template react and then, without installing any other dependency: yarn add git+https://github.com/IBM/mobx-react-router.git#master All that on vite 5.0.8, yarn 1.22.10 and node 18.19.0.

The way I imported the class was:

 import MobxReactRouter from '@ibm/mobx-react-router';

class Router extends MobxReactRouter {}

export default Router;

The entire error goes like this:

Error:   Failed to scan for dependencies from entries:
  /Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/index.html

  ✘ [ERROR] Failed to resolve entry for package "@ibm/mobx-react-router". The package may have incorrect main/module/exports specified in its package.json. [plugin vite:dep-scan]

    node_modules/esbuild/lib/main.js:1374:21:
      1374 │         let result = await callback({
           ╵                      ^

    at packageEntryFailure (file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:29443:17)
    at resolvePackageEntry (file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:29440:5)
    at tryNodeResolve (file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:29210:20)
    at Context.resolveId (file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:28978:28)
    at Object.resolveId (file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:63987:64)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async resolve (file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:64295:26)
    at async file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:64480:34
    at async requestCallbacks.on-resolve (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:1374:22)
    at async handleRequest (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:732:11)

  This error came from the "onResolve" callback registered here:

    node_modules/esbuild/lib/main.js:1293:20:
      1293 │       let promise = setup({
           ╵                     ^

    at setup (file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:64470:19)
    at handlePlugins (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:1293:21)
    at buildOrContextImpl (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:979:5)
    at Object.buildOrContext (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:788:5)
    at /Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:2223:68
    at new Promise (<anonymous>)
    at Object.context (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:2223:27)
    at Object.context (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:2048:58)
    at prepareEsbuildScanner (file:///Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/vite/dist/node/chunks/dep-9A4-l-43.js:64247:26)

  The plugin "vite:dep-scan" was triggered by this import

    src/Router.js:1:28:
      1 │ import MobxReactRouter from '@ibm/mobx-react-router';
        ╵                             ~~~~~~~~~~~~~~~~~~~~~~~~

    at failureErrorWithLog (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:1651:15)
    at /Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:1059:25
    at runOnEndCallbacks (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:1486:45)
    at buildResponseToResult (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:1057:7)
    at /Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:1069:9
    at new Promise (<anonymous>)
    at requestCallbacks.on-end (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:1068:54)
    at handleRequest (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:732:17)
    at handleIncomingPacket (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:757:7)
    at Socket.readFromStdout (/Users/kamil/dev/moje/mobxRouterTest/ReactMobxRouterTest/node_modules/esbuild/lib/main.js:680:7)
gcattan commented 7 months ago

Ok, I see. The package is installed from Github, but actually is never built. A quick fix is to open the package in the node_modules and change the module property of package.json to index.js:

image

kkollman commented 7 months ago

@gcattan Thank you! It seems, however, that this approach isn't too stable - any reinstall causes further issues. At some point I experienced new bug: Transforming JavaScript decorators to the configured target environment ("chrome87", "edge88", "es2020", "firefox78", "safari14" + 2 overrides) is not supported yet

But I think I will simply wait for the stable release of the new version of the lib. Thank you @gcattan for the time you spent on it! :)

gcattan commented 7 months ago

@kkollman No worries. I pushed the changes in progress to another branch and released v6.0.0-alpha on NPM. It seems that vite.js is able to start with this new version. Give it a try when you have time ;)

kkollman commented 7 months ago

@gcattan thank you! 🫡 ❤️

I can confirm installation works well and you can start app with no problem. The only caveat is that the router store, when initiated on its own (i.e. via const routingStore = new MobxRouter) has both location and history set to null. Which means we are back to square one :)

The installation and usage docs are now outdated - not compatible with the react-router v6 way of creating and providing the router/history - <Router> component doesn't accept history prop anymore.

gcattan commented 7 months ago

Hm. I would try something like this:

import { Router  } from 'react-router';

routerStore = new RouterStore();
memoryHistory = createMemoryHistory('/');
history = syncHistoryWithStore(memoryHistory, routerStore);
...
<Router 
      location={history.location}
      navigator={history}
/>
...
kkollman commented 7 months ago

Thanks! I tried a couple of approaches based on your answer. I eventually used createBrowserHistory instead of memory history, and I can see the url change and the observed location property in the store changes as well. The <Routes> however, do not seem to update and UI stays the same even after clicking nav items. The app layout changes accordingly to the pathname only on refresh, as if the Router only provided the initial location and didn't update it on further navigation events. I made sure both App and Routes components are wrapped in mobx observer() but that doesn't fix the issue.

It feels like we are really close :)

My current architecture is somewhat this:

//App Container:
const myHistory = createBrowserHistory();

const AppContainer = () => {
  const rootStore = useStores();
  const { appStore, authStore, routingStore } = rootStore;

  const appHistory = syncHistoryWithStore(myHistory, routingStore);

  return (
    <Provider {...rootStore}>
      <IntlProvider>
        <Router
          location={appHistory.location}
          navigator={appHistory}
        >
          <App/>
        </Router>
      </IntlProvider>
    </Provider>
  );
};

export default observer(AppContainer);

//App component -> it doesn't seem to update on navigation events

const App = () => {
  return (
    <>
      <Helmet>...
      </Helmet>
      {isPending && (
        <div
          className={
            'flex h-full min-h-[100vh] w-full items-center justify-center bg-secondary-blue-100'
          }
        >
          <LoadingSpinner />
        </div>
      )}
      <Routes />}
    </>
  );
};

export default observer(App);
gcattan commented 7 months ago

Hi, sorry for the mistake. This location property inside the browserHistory is not observable, but the one inside the store is.

Something like this should work better ^^'

...
<Router 
      location={routingStore.location}
      navigator={appHistory}
/>
...
kkollman commented 7 months ago

Good point. That being said, with approach you suggested, react is re-rendering endlessly, perhaps that's something nested deeper in my App. Removing observer the component where Router is placed helps it, but then navigation is not working again 🙃 I think I will have to find a workaround - at least I found a way to expose the router navigation methods to the store:


export class RoutingStore {
  router;

  constructor() {
    makeObservable(this, {
      router: observable,
    });
    this.router = null;
  }

  setRouter(router) {
    this.router = router;
  }
}

const router = createBrowserRouter([
  {
    path: '*',
    element: <MyRoutes />,
  },
]);

And then:
routingStore.setRouter(router);
gcattan commented 7 months ago

Cool, we are making some progress :) I finally was able to set up the stackblitz, with a simple example.

https://stackblitz.com/edit/github-bje76z-uyn64v?file=src%2Fmain.jsx

If you have some time, can you try to see if you can reproduce your bug there?

kkollman commented 7 months ago

Okay, I can finally say: done! 🎉 The last missing puzzle was to properly configure vite.js plugins. Now I can say your solution is fully working :) Thank you very much! It's been a while, but we finally made it :)

gcattan commented 7 months ago

Super! Thank you very much for your patience and your help :) I am raising a PR to fix the doc: https://github.com/IBM/mobx-react-router/pull/190/files

Do you see other details that need to be there?

kkollman commented 7 months ago

It looks good - in the docs I would also recommend adding some info on vite plugins to enable @decorators, since not all mobx users will have them added out of the box. I think the newer versions of mobx discarded using those completely. Perhaps it would be advisable to update that syntax in your lib, too? That way you would avoid unnecessary complexity.

gcattan commented 7 months ago

Ok, I have updated the documentation with a example of config file. I think you made a good point about the decorators.

Let's put it into the roadmap https://github.com/IBM/mobx-react-router/issues/191