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

refactor: merge the routers #3

Merged
merged 35 commits into from
Oct 16, 2019
Merged

refactor: merge the routers #3

merged 35 commits into from
Oct 16, 2019

Conversation

mikemurray
Copy link
Member

@mikemurray mikemurray commented Oct 11, 2019

Resolves #1
Impact: major
Type: refactor

Issue

Merge the two routers into one simpler router

Solution

  • Add a registerRoute method to register public routes, like login/auth routes
  • Disable the old router from starting up
  • Create a hook for react-router useRouter to help
  • Create a hook to help with some auth/permissions checks useAuth
  • Add a base App component that is the overall App component that switches between public/admin routes.
  • Public routes are rendered in the App without an auth check, admin routes are Rendered in the Dashboard component like usual
  • Register routes for app auth (meteor) and hydra

Breaking changes

Testing

  1. Use the app normally
  2. (sign in, sign out, navigate to various places in the admin)
  3. Sign out on different pages, then sign back in

Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
@mikemurray
Copy link
Member Author

Here are some places where I had to hardcode the path to the API. None of this was committed, but these values will need to be added to some kind of env config so they can be changed.

diff --git a/client/modules/i18n/startup.js b/client/modules/i18n/startup.js
index 23d5d00b4..fe1f54797 100644
--- a/client/modules/i18n/startup.js
+++ b/client/modules/i18n/startup.js
@@ -37,7 +37,7 @@ const configuredI18next = i18next
 async function initializeI18n(fallbackLng) {
   // Reaction does not have a predefined list of namespaces. Any API plugin can
   // add any namespaces. So we must first get the list of namespaces from the API.
-  const namespaceResponse = await fetch("/locales/namespaces.json");
+  const namespaceResponse = await fetch("http://localhost:3000/locales/namespaces.json");
   const allTranslationNamespaces = await namespaceResponse.json();
 
   try {
@@ -46,7 +46,7 @@ async function initializeI18n(fallbackLng) {
         backend: i18nextFetch,
         backendOption: {
           allowMultiLoading: true,
-          loadPath: "/locales/resources.json?lng={{lng}}&ns={{ns}}"
+          loadPath: "http://localhost:3000/locales/resources.json?lng={{lng}}&ns={{ns}}"
         }
       },
       debug: false,
diff --git a/imports/plugins/core/graphql/lib/helpers/initApollo.js b/imports/plugins/core/graphql/lib/helpers/initApollo.js
index 2a20832da..5e0b6107e 100644
--- a/imports/plugins/core/graphql/lib/helpers/initApollo.js
+++ b/imports/plugins/core/graphql/lib/helpers/initApollo.js
@@ -6,7 +6,7 @@ import { InMemoryCache } from "apollo-cache-inmemory";
 import { getOperationAST } from "graphql";
 import { Reaction } from "/client/api";
 
-const httpUrl = Reaction.absoluteUrl("graphql-beta");
+const httpUrl = "http://localhost:3000/graphql-beta";
 const wsUrl = httpUrl.replace("http", "ws");
 
 export const meteorAccountsLink = new ApolloLink((operation, forward) => {
diff --git a/imports/plugins/core/graphql/lib/helpers/simpleClient.js b/imports/plugins/core/graphql/lib/helpers/simpleClient.js
index 713b909c0..bd6b36bc7 100644
--- a/imports/plugins/core/graphql/lib/helpers/simpleClient.js
+++ b/imports/plugins/core/graphql/lib/helpers/simpleClient.js
@@ -12,7 +12,7 @@ import paymentMethods from "../queries/paymentMethods.graphql";
  * components and other code, but ideally we will not need this forever.
  */
 
-const client = graphql(Reaction.absoluteUrl("graphql-beta"), { asJSON: true });
+const client = graphql("http://localhost:3000/graphql-beta", { asJSON: true });
 
 /**
  * @summary Sets the meteor-login-token header for all GraphQL requests done

Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
@@ -46,7 +47,7 @@ async function initializeI18n(fallbackLng) {
backend: i18nextFetch,
backendOption: {
allowMultiLoading: true,
loadPath: "/locales/resources.json?lng={{lng}}&ns={{ns}}"
loadPath: i18nResourceUrl
Copy link
Contributor

Choose a reason for hiding this comment

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

@mikemurray Since these routes are not configurable on the API side, it seems safe and simpler to manage to have just a single i18nBaseUrl in config and append the rest here

// Attempt to check if we are still loading this data
setLoading((hasDashboardAccessForAnyShop !== true && hasDashboardAccessForAnyShop !== false) || !shop);

if (!hasStorefrontHomeUrl && !isLoading) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should fall back to a storefrontHomeUrl from config and if that also isn't set, then show the warning.

* Hook to get user permissions for the App component
* @return {Object} Permissions
*/
export default function useHasAdminAccess() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Name should be useAuth to match file name

Meteor.logout((error) => {
if (error) Logger.error(error);
setLoggingOut(false);
});
Copy link
Contributor

@aldeed aldeed Oct 12, 2019

Choose a reason for hiding this comment

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

@mikemurray I think it's a little cleaner to move this onClick function to useAuth hook, and rather than pass down setLoggingOut, just pass down logOut. Then call logOut() here in the onClick. It hides and centralizes the Meteor dependency.

@@ -27,15 +24,15 @@ const link = ApolloLink.split(
return !!operationAST && operationAST.operation === "subscription";
},
new WebSocketLink({
uri: wsUrl,
uri: graphQlWebSocketUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add the WebSocketLink only if graphQlWebSocketUrl is set in config. That's a convenient way of disabling subscriptions.


const httpUrl = Reaction.absoluteUrl("graphql-beta");
const wsUrl = httpUrl.replace("http", "ws");
import { graphQlApiUrl, graphQlWebSocketUrl } from "/config";
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a good place to check and throw a helpful error message if graphQlApiUrl isn't set in config.

Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
@mikemurray mikemurray changed the title [WIP] refactor: merge the routers refactor: merge the routers Oct 16, 2019
@mikemurray mikemurray marked this pull request as ready for review October 16, 2019 17:32
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
Signed-off-by: Mike Murray <hellomikemurray@gmail.com>
@aldeed aldeed merged commit ff3f2c9 into trunk Oct 16, 2019
@aldeed aldeed deleted the refactor-1-merge-the-routers branch October 16, 2019 23:30
@kieckhafer kieckhafer mentioned this pull request Feb 6, 2020
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.

Remove old router
2 participants