Skip to content

Fix DynamicModuleLoader and remove support for React-Redux 5 #77

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 9, 2019
Merged

Fix DynamicModuleLoader and remove support for React-Redux 5 #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b35acc9
Fix potential issue with ReactRedux 7
Apr 7, 2019
db512cc
Remove support for RR5
Apr 7, 2019
2fad853
Remove exports
Apr 7, 2019
4cb3321
CR
Apr 29, 2019
b75fbd9
Add more doc comments:
Apr 30, 2019
994c5ae
Merge branch 'master' into users/alebet/RR7
abettadapur May 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/redux-dynamic-modules-react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
},
"peerDependencies": {
"react": ">= 15.0.0",
"react-redux": ">= 5.0.0",
"react-redux": ">= 6.0.0",
"redux": ">= 3.0.0"
},
"dependencies": {
Expand Down
189 changes: 87 additions & 102 deletions packages/redux-dynamic-modules-react/src/DynamicModuleLoader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import * as PropTypes from "prop-types";
import * as React from "react";
//@ts-ignore
//@ts-ignore // ReactReduxContext is not officially exported
import { Provider, ReactReduxContext } from "react-redux";

import {
Expand All @@ -13,16 +12,18 @@ export interface IDynamicModuleLoaderProps {
/** Modules that need to be dynamically registerd */
modules: IModuleTuple;

/**
* Set this flag to indicate that this component is being rendered in 'Strict Mode'
* React 'StrictMode' does not allow constructor side-effects, so we defer adding modules to componentDidMount
* when this flag is set.
* This has the effect of adding a second render.
*/
strictMode?: boolean;

/** Optional callback which returns a store instance. This would be called if no store could be loaded from the context. */
/** Optional callback which returns a store instance. This would be called if no store could be loaded from th e context. */
createStore?: () => IModuleStore<any>;
}

export interface IDynamicModuleLoaderContext {
store: IModuleStore<any>;
}

/**
* The DynamicModuleLoader adds a way to register a module on mount
* When this component is initialized, the reducer and saga from the module passed as props will be registered with the system
Expand All @@ -31,81 +32,52 @@ export interface IDynamicModuleLoaderContext {
export class DynamicModuleLoader extends React.Component<
IDynamicModuleLoaderProps
> {
// @ts-ignore
private static contextTypes = {
store: PropTypes.object,
};

constructor(
props: IDynamicModuleLoaderProps,
context: IDynamicModuleLoaderContext
) {
super(props, context);
}

/**
* Render a Redux provider
*/
public render(): React.ReactNode {
if (ReactReduxContext) {
return (
<ReactReduxContext.Consumer>
{context => {
return (
<DynamicModuleLoaderImpl
createStore={this.props.createStore}
store={context ? context.store : undefined}
strictMode={this.props.strictMode}
modules={this.props.modules}>
{this.props.children}
</DynamicModuleLoaderImpl>
);
}}
</ReactReduxContext.Consumer>
);
} else {
return (
<DynamicModuleLoaderImpl
// @ts-ignore
createStore={this.props.createStore}
store={this.context.store}
strictMode={this.props.strictMode}
modules={this.props.modules}>
{this.props.children}
</DynamicModuleLoaderImpl>
);
}
public render() {
return (
<ReactReduxContext.Consumer>
{reactReduxContext => (
<DynamicModuleLoaderImpl
{...this.props}
reactReduxContext={reactReduxContext}
/>
)}
</ReactReduxContext.Consumer>
);
}
}

interface IDynamicModuleLoaderImplProps {
/** Modules that need to be dynamically registerd */
modules: IModuleTuple;

store: IModuleStore<any>;

strictMode: boolean;

createStore?: () => IModuleStore<any>;
interface IDynamicModuleLoaderImplProps extends IDynamicModuleLoaderProps {
/** The react-redux context passed from the <Provider> component */
reactReduxContext?: { store: IModuleStore<any> };
Copy link
Contributor

Choose a reason for hiding this comment

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

doc comments.

}

interface IDynamicModuleLoaderImplState {
/** Is the DML component ready to render.
* If strictMode is set to false, this will be set to true initially
* If strict mode is set to true, this will be set after the first render completes
*/
readyToRender: boolean;
}

class DynamicModuleLoaderImpl extends React.Component<
IDynamicModuleLoaderImplProps,
IDynamicModuleLoaderImplState
> {
/** The modules that were added from this loader */
private _addedModules?: IDynamicallyAddedModule;
/** Flag that indicates we need to create a store/provider because a parent store was not provided */
private _providerInitializationNeeded: boolean = false;
/** The module store, derived from context */
private _store: IModuleStore<any>;
private _getLatestState: boolean;
/** The react redux context, saved */
private _memoizedReactReduxContext: any;

constructor(props: IDynamicModuleLoaderImplProps) {
super(props);

this._store = this.props.store;
this._store = props.reactReduxContext
? props.reactReduxContext.store
: undefined;

// We are not in strict mode, let's add the modules ASAP
if (!this.props.strictMode) {
Expand All @@ -118,60 +90,73 @@ class DynamicModuleLoaderImpl extends React.Component<
}
}

private _addModules(): void {
const { createStore, modules } = this.props;
if (!this._store) {
if (createStore) {
this._store = createStore();
this._providerInitializationNeeded = true;
} else {
throw new Error(
"Store could not be resolved from React context"
public render(): React.ReactNode {
if (this.state.readyToRender) {
Copy link
Contributor

Choose a reason for hiding this comment

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

comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What comments are you looking for here? This seems pretty self explanatory to me

if (this._providerInitializationNeeded) {
return (
<Provider store={this._store}>
{/* We just rendered the provider, so now we need to render
DML again. This one will add the modules */}
<DynamicModuleLoader {...this.props} />
</Provider>
);
}
} else {
// We will add modules dynamically and due to github issue https://github.com/Microsoft/redux-dynamic-modules/issues/27#issuecomment-464905893
// The very first render will not get latest state, to fix that we will need to get latest state from store directly on first render
this._getLatestState = ReactReduxContext;

return this._renderLoader();
}

this._addedModules = this._store.addModules(modules);
return null;
}

private _renderWithReactReduxContext = () => {
const { store } = this.props;
// store.getState is important here as we don't want to use storeState from the provided context
/**
* Render a Redux provider
*/
private _renderLoader(): React.ReactNode {
if (this.props.reactReduxContext == null) {
const message =
"Tried to render DynamicModuleLoader, but no ReactReduxContext was provided";
console.error(message);

throw new Error(message);
}

// Memoize the context if it has changed upstream
// If the context has not changed, we want to use the same object reference so that
// downstream consumers do not update needlessly
if (this.props.reactReduxContext !== this._memoizedReactReduxContext) {
this._memoizedReactReduxContext = {
...this.props.reactReduxContext,
storeState: this.props.reactReduxContext.store.getState(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we memoizing state only when context changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the state changes above, then we will get a new context object here.

See: https://github.com/reduxjs/react-redux/blob/162b81a3fffe75ff6181b711777067ba6e63a34b/src/components/Provider.js#L59

this.setState({...}) returns a new object

};
}

return (
<ReactReduxContext.Provider
value={{ store, storeState: store.getState() }}>
{this._renderChildren()}
<ReactReduxContext.Provider value={this._memoizedReactReduxContext}>
{this.props.children &&
typeof this.props.children === "function"
? this.props.children()
: this.props.children}
</ReactReduxContext.Provider>
);
};

private _renderChildren = () => {
if (this.props.children && typeof this.props.children === "function") {
return this.props.children();
}
}

return this.props.children;
};
private _addModules(): void {
const { createStore, modules } = this.props;

public render(): React.ReactNode {
if (this.state.readyToRender) {
if (this._providerInitializationNeeded) {
return (
<Provider store={this._store}>
{this._renderChildren()}
</Provider>
if (!this._store) {
// If we need to create a store, do that here. We will skip adding the modules and render DML again
if (createStore) {
this._store = createStore();
this._providerInitializationNeeded = true;
} else {
throw new Error(
"Store could not be resolved from React context"
);
} else if (!this._getLatestState) {
return this._renderChildren();
}

return this._renderWithReactReduxContext();
} else {
// Add the modules here
this._addedModules = this._store.addModules(modules);
}
return null;
}

public componentDidMount() {
Expand Down