nebarf / module-federation-react-router-dom

Module federation example using nested routers
https://github.com/nebarf/module-federation-react-router-dom
41 stars 16 forks source link

Module not found: Error: Can't resolve 'history' in '/app/src' #1

Closed Jamison1 closed 1 year ago

Jamison1 commented 2 years ago

Issue: The project uses react-router-dom v6.3 which removed history as a dependency.

When creating node modules from scratch using package.json file the history module is not installed which throws the above error from the "app1/app2 > src > bootstrap.tsx" & "shell > src > routing > constants.ts" files.

In looking at documentation for react-router-dom upgrade from v5 to v6 it mentions removing history from the package.json as an upgrade step:

Upgrade to v6 Guide

"You'll also want to remove the history dependency from your package.json. The history library is a direct dependency of v6 (not a peer dep), so you won't ever import or use it directly. Instead, you'll use the useNavigate() hook for all navigation"

By chance did your node_modules still contain the old reference to the history module enabling the old import from 'history' to work?

This is a nice implementation of the MFE course from Stephen Grider.

Although you could install the history module v5.3 as a hack - since RRD v6 is designed to work with useNavigate() have you found a way to get the history to work with upgraded navigation without reinstalling the history module?

nebarf commented 2 years ago

@Jamison1 As stated in the upgrade guide the history library is a direct dependency of react-router-dom v6 so it's installed along with react-router-dom. I just cloned the repo, installed deps and I was able to find the history lib under node_modules dir.

Moreover explicitly installing the history package is discouraged. Quoting from the docs:

It's important to note that using your own history object is highly discouraged and may add two versions of the history library to your bundles unless you use the same version of the history library that React Router uses internally.

Jamison1 commented 2 years ago

Yes, agree that's what it says. So if you delete the package-lock.json file in the repo after cloning the repo and reinstall the node_modules from the package.json file only (from scratch) - let me know if you still see a history module as a peer dependency? Mine doesn't have one. Causing the error listed in the title.

There is an older upgrade doc (I haven't had time to find it - maybe upgrade from v5 to v6.0-beta) that gives an additional step of using npm uninstall history after removing the history module from the package.json file. This would remove the history module as a peer dependency from both the package-lock.json and node_modules. Then when installing react-router-dom v6 the history module would be installed correctly as a direct dependency.

But this still doesn't solve the issue:import {...} from 'history' where the upgrade document states: "so you won't ever import or use it (meaning 'history') directly". Am I reading the docs incorrectly here?

Removing theimport {...} from 'history' appears to be one of the upgrade steps according to the docs. I count 7? imports from 'history' that need replaced with the new navigation from RRD v6.

So this is why my original post asked: "have you found a way to get the history to work with upgraded navigation without reinstalling the history module"?

I am currently upgrading another project to latest RRD v6 without any import {...} from 'history' in accordance with the upgrade docs and was curious if you had a chance to complete that step.

In looking through all of the comments from the students of the "Microfrontends with React: A Complete Developer's Guide" which this repo is based on - it appears that all of the students were struggling with this last step and still trying to figure out how to remove the import {...} from 'history' during their upgrade process.

If upgrading RRD from v5 to v6 without running npm uninstall history then the history module appears to still be in the node_modules or package-lock.json file as a peer dependency and the import statement works. If installing RRD v6 from scratch - the history module is not installed as a peer dependency - causing the error in the title.

The conclusion from the students seems to be "just npm install history" again after upgrading to v6 of RRD and keep the import {...} from 'history'.

This "hack" does work but doesn't use the new navigation built in to RRD v6 which requires that you remove import {...} from 'history'.

This is an issue with MFE using module federation and RRD v6 and I am looking for a way around it without importing directly from the history module which was a RRD v5 solution.

nebarf commented 1 year ago

Just to be more accurate, history package was not a peer dependency but rather a direct dependency of react-router-dom.

Having said that, I took a deeper look at library changes and found that the history lib was removed from the dependency list as part of v6.4.0 release (look at the commit).

So starting from v6.4.0 you won't find any history package under node_modules and the only way to make it working it's by manually installing history dependency. Since the usage of an external history object is discouraged, I'd like to find an alternative solution that makes use of new native API exposed by react-router-dom. Will work on it as soon as I'll ve enough time.

In the mean time, if you find a replacement solution please let me know. Feel free to open a PR against this repo.

nebarf commented 1 year ago

Opened a draft PR

Jamison1 commented 1 year ago

In researching this it seems there are 2 issues with 6.4 in upgrading the original course code that still need solved: The first is history.listen(onNavigate). The listen function is no longer available. The second is history.push('nextPathname'). The replacement hook useNavigate() for navigate('nextPathname') doesn't seem to be a direct replacement forhistory.push('nextPathname'). Examples are taken from the original course code referenced in the README.md.

The new data api's in 6.4 are promising with createBrowserRouter & createMemoryRouter if you can find a solution for the 2 issues referenced.

nebarf commented 1 year ago

Based on latest docs the preferred way of performing side effects on location changes is using the useLocation hook.

I refactored the repo by using new API exposed by react-router-dom.

@Jamison1 You can navigate the code by looking at fix/unstable-history-router branch. I'll wait for merge it since it's not battle-tested, please let me know if you have any concerns or it sounds good to you.

Thanks!

Jamison1 commented 1 year ago

Nice work! So far so good with testing. Will continue to test over the next week or so.

Containerized with Node 18.10.0-alpine for testing. Looks like react-router-dom was just upgraded to 6.4.3 which is what loaded in the node_modules.

Some comments here so I don’t clutter with more open issues. You can just address them with an additional comment if you feel the need.

1) Upon first load (I changed the ports) the shell URL reads: http://localhost:8090/app-1/ But the page says Page 1 from App1. If clicking on Page 2 and back to Page 1 the shell URL reads: http://localhost:8090/app-1/page-1 (notice the page-1) and the page says Page 1 from App1.

This appears to be from the children routes having bothindex: true and path: page-1 for element: <Page1 /> Tested and browser bookmarks seem to work fine either way if page-1 was the landing page.

2) In shell>src>components>App1.tsx import { mount } from "app1/App1Index";

and App2.tsx import { mount } from "app2/App2Index";

Get a linting error: Cannot find module 'app1/App1Index' 'app2/App2Index'or its corresponding type declarations.ts(2307)

Maybe needs a custom.d.ts file in shell root with:

declare module 'app1/App1Index'
declare module 'app2/App2Index'

3) And in the same 2 files referenced maybe a cut and paste error?: App1.tsx

// Listen to navigation events dispatched inside app2 mfe. useEffect(() => { const app2NavigationEventHandler = (event: Event) => {

App2.tsx

// Listen to navigation events dispatched inside app2 mfe. useEffect(() => { const app2NavigationEventHandler = (event: Event) => {

4) In shell>src>routing>Router.tsx Import unstable_HistoryRouter as HistoryRouter, needs removed.

nebarf commented 1 year ago

@Jamison1 Thanks a lot for the detailed testing and code review.

Below answers point by point.

  1. I can confirm that the behaviour it's due to index: true config. Keep into account that the repo code it's just a skeleton acting as starting point, of course in a real project the routing has to be adjusted depending on needs.
  2. Correct, using a custom declaration file would make ts compiler of vscode happy. An alternative would be using TS project refs.
  3. Copy-paste errors for sure, thanks for pointing them out.

PR merged to main branch. Please let me know in case you have further thoughts.

Jamison1 commented 1 year ago

@nebarf I've been testing the new code and ran into an issue and not sure how to solve it.

When changing the children path in app1 to only have a "page-1" and "page-3" (eliminate page 2 completely and update all references from page-2 to page-3 in app1 and shell)

app1>src>routing>routes.tsx
...
- path: "page-2",
+ path: "page-3",

there is a console warning upon navigating to app2/page-2: No routes matched location "/page-2".
Upon navigating back to app1/page-3 there is another console warning: No routes matched location "/page-3".

Navigation works fine in both scenarios.

app1>src>routing>routes.tsx
path: "/",
    element: (
      <NavigationManager>
        <Outlet /> # <== is issue here?
      </NavigationManager>
    ),

I am not that familiar with the NavigationManager for App1 but I'm wondering if this is caused by both routes for app1 and app2 being combined with createMemoryRouter.

So app2 is expecting a child route for app2/page-3 and app1 is expecting a child route for app1/page-2? If so is there a way to separate the routes so that each app only references its own routes?

nebarf commented 1 year ago

@Jamison1 good catch thanks! Will take a look at it.

Could you file a new issue? So we can keep track of it.