Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MINOR] shared routing for subapps #1783

Merged
merged 11 commits into from
Jan 14, 2021
Merged

[MINOR] shared routing for subapps #1783

merged 11 commits into from
Jan 14, 2021

Conversation

durrab
Copy link
Contributor

@durrab durrab commented Jan 12, 2021

No description provided.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Durrab Khan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jchip
Copy link
Member

jchip commented Jan 13, 2021

update description comments for options.history to indicate the new behavior

use hasOwnProperty to check if user specified options.history

@@ -0,0 +1,4 @@
import { createBrowserHistory } from "history";
export default class RouterHistory {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit overkill to create a static value, and no export default

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this will trigger createBrowserHistory immediately when app's loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

* - Otherwise it's assumed to be a history object and `Router` will be used with it.
* - If it's false, then `BrowserRouter` will be used and it will use its own history.
* - Note: BrowserRouter use its own history object and its not shared among other subapps
* - If it's `true`, then `Router` is used with history from `createBrowserHistory` from https://www.npmjs.com/package/history and the same history object will be shared among all subapps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if history is not specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this here in the last line

    • Otherwise it's assumed to be a history object and Router will be used with it. If its undefined then share history object will be used for all subapps

@@ -1,9 +1,9 @@
import { SubAppDef, SubAppFeatureFactory } from "@xarc/subapp";
import { Router, BrowserRouter } from "react-router-dom";
import { createBrowserHistory } from "history";

//import RouterHistory from "./router-history"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import { ReactRouterFeatureOptions, _id, _subId } from "../common";

import { createBrowserHistory } from "history";
const reactRouterHistory = createBrowserHistory();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only create it when used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to have reactRouterHistory on top because reactRouterFeature called multiple times for each sub-app and this is a one time call as shared history object

@@ -52,7 +52,7 @@ export const subapp: ReactSubApp = {
reduxFeature({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are these changes in this file for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the subapp-web which was not required here - I used it when I was playing with that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does these changes have to do with subapp-web?

@@ -1,146 +0,0 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo this

TheRouter = BrowserRouter;
} else {
history = options.history === true ? createBrowserHistory() : options.history;
history = options.history === true ? reactRouterHistory : (options.hasOwnProperty("history") ? options.history : reactRouterHistory);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed, the history created in this file is used only if:

  1. options.history is true
  2. options.history is not defined
    Otherwise,
  3. if it's not falsy, assume it's a valid history object and use it with Router
  4. finally use BrowserRouter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no double ?: please

@jchip jchip merged commit ef16d91 into electrode-io:master Jan 14, 2021
jchip pushed a commit that referenced this pull request Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants