accounts-js / react

React (and React Native) view layer for js-accounts
MIT License
7 stars 6 forks source link

Redesign #42

Open pradel opened 7 years ago

pradel commented 7 years ago

Opened this issue to talk about the redesign of the react package. @TimMikeladze @davidyaha

pradel commented 7 years ago

Right now the react and react-native diverge a lot. The idea is to write a common logic shared between the 2 packages. What we need to support:

Idealy we should try to make the packages as light as possible

pradel commented 7 years ago

Here are my thought about how can work the AccountsProvider.

AccountsProvider will forward accountsClient to childs with the react context api.

When AccountsProvider is mounted set redux loading state to true. We check if there are token in the storage if no set loading to false and user to null. If yes try to see if there are valid with the server. If they are valid set loading to false and user. (this part will be handled by an AccountsClient function)

The LogInForm form component will take the config from react context and display the form with the form type (USERNAME_AND_EMAIL etc..) defined in accountsClient config.

If we let the user define the routes, the components can works with any router. The question is how to handle the redirect between the pages from 'sign-in' to 'log-in'. For example react-router use Link component or route.push('route') api.

import { AccountsClient } from 'js-accounts/client';
import { AccountsProvider } from 'js-accounts/react';
import { SignInForm, LogInForm } from 'js-accounts/react-material-ui';

const accountsClient = new AccountsClient(config);

// For react-router v3
const requireAuth = (replace, next) => {
  if (!accountsClient.isLoggedIn()) {
  // or maybe subscribe to the redux store if the app is still loading and call next when the app is ready
    replace('/login-in');
 }
 next();
}

render((
  <AccountsProvider accountsClient={accountsClient}>
    <Router history={browserHistory}>
      <Route path="login" component={LogInForm} />
      <Route path="sign-up" component={SignInForm} />
      <Route path="protected" component={MyProtectedComponent} onEnter={requireAuth} />
    </Router>
  </AccountsProvider>
), document.getElementById('root'));

// For react-router v4
const PrivateRoute = ({ component: Component, ...rest }) => (
  <Route {...rest} render={props => (
    accountsClient.isLoggedIn() ? (
      <Component {...props}/>
    ) : (
      <Redirect to={{
        pathname: '/login',
        state: { from: props.location }
      }}/>
    )
  )}/>
)

render((
  <AccountsProvider accountsClient={accountsClient}>
    <Router>
    <div>
      <ul>
        <li><Link to="/public">Public Page</Link></li>
        <li><Link to="/protected">Protected Page</Link></li>
      </ul>
      <Route path="/login" component={LogInForm}/>
      <PrivateRoute path="/protected" component={Protected}/>
    </div>
  </Router>
  </AccountsProvider>
), document.getElementById('root'));
TimMikeladze commented 7 years ago

The internalization should be handled within the accounts core package because 1) validation messages originate there and 2) we don't want to write additional internalization code for other ui libraries (vue, angular). One thing that we do need to define is the strings for field labels and other ui pieces like "Forgot Password".

If we let the user define the routes, the components can works with any router. The question is how to handle the redirect between the pages from 'sign-in' to 'log-in'. For example react-router use Link component or route.push('route') api.

I think this can be simply solved by allowing the user to pass in a function which defines what should occur on route changes. I'd like to avoid tying this package to any specific router but we can provide recipes or even a helper function which will define the default routes if a user so chooses.

Two important pieces of functionality that I'd like to see possible

  1. Custom forms and fields
  2. Multi step forms - think of a signup form where the user is taken through multiple steps.

These can be defined by the user and then override the existing default forms when they configure the AccountsProvider.

pradel commented 7 years ago

I think this can be simply solved by allowing the user to pass in a function which defines what should occur on route changes. I'd like to avoid tying this package to any specific router but we can provide recipes or even a helper function which will define the default routes if a user so chooses.

This can work yes. We can provide some examples in the docs for major routers.

Custom forms fields and multi step forms looks like they will add a lot of complexity to the forms part right now no?

TimMikeladze commented 7 years ago

@pradel I don't think custom forms will add much complexity, after all they are written by the user and passed in as a prop. What is needed on our end is to make sure we allow the user to pass in this prop.

davidyaha commented 7 years ago

Hey, sorry for my late response.

I like the direction you're going in.

About custom forms, let's try to elaborate what are the reasons for writing custom/multi step forms. From what I figured:

For starters, these could probably be achieved by the user by just using the "AccountsProvider" and implement his own custom form.

Do you think of any other use case?

TimMikeladze commented 7 years ago

We have a use case in our app at work where the login form has a select field with a list of different authentication servers the user can send the login request to.

davidyaha commented 7 years ago

So in order to support that you actually need multiple AccountsClient instances or you call config multiple times right?

TimMikeladze commented 7 years ago

The way I accomplish it right now is below. Not the best way to handle it but it works for now.

export const restClient = new RestClient({
  apiHost: process.env.CONTROLLER,
  rootPath: '/accounts',
});
setAuthServer(id, url) {
    localStorage.setItem(localStorageItemId, id);
    localStorage.setItem(localStorageItemUrl, url);
    // TODO This is a hack. The client doesn't support setting the apiHost from a function
    restClient.options.apiHost = url;
  }

Ideally I'd just like to initialize a new rest client, pass it to a new accounts instance which is turn passed to the AccountsProvider

pradel commented 7 years ago

For the forms:

I think each ui packages should handle the update logic and call accountsClient.validate which will return an array of errors that the ui-package can show to the user.

By default it will have default className but the user will also be able to override some props:

<SignInForm layoutClassName="my-layout" errorClassName="error" ... /> 

Components needed:

TimMikeladze commented 7 years ago

@pradel That sounds good. I think we should write a change password form (or maybe just the fields) since I believe we provide a changePassword mutation in the api.

davidyaha commented 7 years ago

The list of forms looks good! The className approach won't work react-native unfortunately.. We should maybe use style props instead? From what i've seen those work for both react-native and react. Are there any disadvantages to style that you know of?

pradel commented 7 years ago

In react we can't do media-queries with style approach but we can do style + classname for react and style only for react-native since it will be handled by the differents ui packages