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

Replacing 'routerReducer' with a configurable option #417

Merged
merged 17 commits into from
Oct 23, 2017

Conversation

phillipzada
Copy link
Contributor

See #410

  • Creation of a forRoot method to allow for configuration of StoreRouterConnectingModule
  • Replacement of 'routerReducer' static state key with a configurable value

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 4583010 on PhillipZada:master into ** on ngrx:master**.

@@ -1,3 +1,4 @@
import { StoreRouterConfig } from '../src/router_store_module';
Copy link
Member

Choose a reason for hiding this comment

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

This can be added to the imports below from ../src/index

@@ -155,18 +165,31 @@ export function routerReducer<T = RouterStateSnapshot>(
],
})
export class StoreRouterConnectingModule {
Copy link
Member

Choose a reason for hiding this comment

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

This would be a breaking change for existing users unless you add default providers to to NgModule metadata

static forRoot(config: StoreRouterConfig = {}): ModuleWithProviders {
return {
ngModule: StoreRouterConnectingModule,
providers: [{ provide: _ROUTER_CONFIG, useValue: config }],
Copy link
Member

Choose a reason for hiding this comment

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

This won't work in AoT because the actual value for useValue property won't get resolved. You'll need to have two tokens, one for internal use that you register using useValue and another you register with useFactory that uses the first token as a dependency. Then you'll inject the public token in the constructor.

private routerState: RouterStateSnapshot;
private storeState: any;
private lastRoutesRecognized: RoutesRecognized;

private dispatchTriggeredByRouter: boolean = false; // used only in dev mode in combination with routerReducer
private navigationTriggeredByDispatch: boolean = false; // used only in dev mode in combination with routerReducer

private stateKey: string = 'routerReducer';
Copy link
Member

Choose a reason for hiding this comment

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

Initialize this value in the factory function used to register the token

@phillipzada
Copy link
Contributor Author

Hey @brandonroberts just wanted to get your thoughts my my previous comments.

@brandonroberts
Copy link
Member

Hey @phillipzada. If you're talking about jasmine-marbles, I just responded to your comments.

@phillipzada
Copy link
Contributor Author

@brandonroberts sorry mate, I was referring the comments to your comments to the code changes above.

Hey Brandon,

Would this work, you do you still prefer an internal and external token?


export function createDefaultRouterConfig(config: StoreRouterConfig = {}): StoreRouterConfig {
  return {
    stateKey: 'routerReducer',
    ...config
  }
}



@NgModule({
  providers: [
    { provide: RouterStateSerializer, useClass: DefaultRouterStateSerializer },
    { provide: ROUTER_CONFIG, useFactory: createDefaultRouterConfig }
  ],
})
export class StoreRouterConnectingModule {
  static forRoot(config?: StoreRouterConfig): ModuleWithProviders;
  static forRoot(config: StoreRouterConfig = {}): ModuleWithProviders {
    return {
      ngModule: StoreRouterConnectingModule,
      providers: [{ provide: ROUTER_CONFIG, useFactory: createDefaultRouterConfig, deps: [config] }],
    };
  }

  private routerState: RouterStateSnapshot;
  private storeState: any;
  private lastRoutesRecognized: RoutesRecognized;

  private dispatchTriggeredByRouter: boolean = false; // used only in dev mode in combination with routerReducer
  private navigationTriggeredByDispatch: boolean = false; // used only in dev mode in combination with routerReducer

  private stateKey: string;

  constructor(
    private store: Store<any>,
    private router: Router,
    private serializer: RouterStateSerializer<RouterStateSnapshot>,
    @Inject(ROUTER_CONFIG) private config: StoreRouterConfig
  ) {
    const routerConfig = createDefaultRouterConfig(config);
    this.stateKey = (routerConfig.stateKey as string);

@brandonroberts
Copy link
Member

@phillipzada changes look good for the most part. You can't use config as a dependency without it being a token though. And then you won't need to call createDefaultRouterConfig in the constructor.

@brandonroberts brandonroberts self-assigned this Sep 28, 2017
@phillipzada
Copy link
Contributor Author

Thanks @brandonroberts, appreciate the assist, will make the changes this week.

@phillipzada
Copy link
Contributor Author

Hey @brandonroberts, Changes have been made.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling cbbe801 on phillipzada:master into ** on ngrx:master**.

@brandonroberts
Copy link
Member

Thanks @phillipzada. One small thing. Change @ngrx/router to @ngrx/router-store in the InjectionToken strings. Everything else looks good.

@brandonroberts
Copy link
Member

The providers in the forRoot method should be in the NgModule metadata also to prevent a breaking change for existing users.

@brandonroberts
Copy link
Member

@phillipzada Can you fix the merge conflicts and failing tests? I'm ready to land this one.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 92.494% when pulling dae762f on phillipzada:master into 565389a on ngrx:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 92.494% when pulling 3e5ff01 on phillipzada:master into 4db7d61 on ngrx:master.

@brandonroberts
Copy link
Member

@phillipzada can you remove the yarn.lock file from the changes? There shouldn't be any dependency changes.

@brandonroberts
Copy link
Member

@phillipzada sorry, I wasn't clear. Can you remove the yarn.lock changes? The yarn.lock file in master should remain unchanged

@phillipzada
Copy link
Contributor Author

So make a copy of the current version and put it back in?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 92.494% when pulling d04b3f6 on phillipzada:master into 4db7d61 on ngrx:master.

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