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.

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

[docs] Remove old workarounds #20587

Merged
merged 5 commits into from
Apr 17, 2020
Merged

[docs] Remove old workarounds #20587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab9302f
chore: remove unused markdownLocation prop
eps1lon Apr 16, 2020
a7030e5
[docs] Remove global redux store
eps1lon Apr 16, 2020
e262548
[docs] Can we remove this dynamic require?
eps1lon Apr 16, 2020
b438d6f
Update docs/src/modules/components/TopLayoutBlog.js
eps1lon Apr 16, 2020
b83ebae
Update docs/src/modules/components/TopLayoutBlog.js
eps1lon Apr 17, 2020
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
20 changes: 4 additions & 16 deletions docs/pages/_app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,9 @@ function AppWrapper(props) {
const { children, pageProps } = props;

const router = useRouter();
const [redux] = React.useState(() => initRedux(pageProps.reduxServerState));
const [redux] = React.useState(() =>
initRedux({ options: { userLanguage: pageProps.userLanguage } }),
);

React.useEffect(() => {
loadDependencies();
Expand Down Expand Up @@ -333,26 +335,12 @@ MyApp.propTypes = {
};

MyApp.getInitialProps = async ({ ctx, Component }) => {
let pageProps = {};
let pageProps = { userLanguage: ctx.query.userLanguage };

if (Component.getInitialProps) {
pageProps = await Component.getInitialProps(ctx);
}

if (!process.browser) {
const redux = initRedux({
options: {
userLanguage: ctx.query.userLanguage,
},
});
pageProps = {
...pageProps,
// No need to include other initial Redux state because when it
// initialises on the client-side it'll create it again anyway
reduxServerState: redux.getState(),
};
}

return {
pageProps,
};
Expand Down
10 changes: 5 additions & 5 deletions docs/src/modules/components/MarkdownDocs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import PageContext from 'docs/src/modules/components/PageContext';
import { getHeaders, getTitle, getDescription } from 'docs/src/modules/utils/parseMarkdown';
import { pageToTitleI18n } from 'docs/src/modules/utils/helpers';
import Link from 'docs/src/modules/components/Link';
import { exactProp } from '@material-ui/utils';

const styles = (theme) => ({
root: {
Expand Down Expand Up @@ -92,7 +93,6 @@ function MarkdownDocs(props) {
disableAd = false,
disableToc = false,
markdown: markdownProp,
markdownLocation: markdownLocationProp,
req,
reqPrefix,
reqSource,
Expand All @@ -102,7 +102,6 @@ function MarkdownDocs(props) {

const markdownDocs = useMarkdownDocs({
markdown: markdownProp,
markdownLocation: markdownLocationProp,
req,
reqPrefix,
reqSource,
Expand Down Expand Up @@ -194,12 +193,13 @@ MarkdownDocs.propTypes = {
disableAd: PropTypes.bool,
disableToc: PropTypes.bool,
markdown: PropTypes.string,
// You can define the direction location of the markdown file.
// Otherwise, we try to determine it with an heuristic.
markdownLocation: PropTypes.string,
req: PropTypes.func,
reqPrefix: PropTypes.string,
reqSource: PropTypes.func,
};

if (process.env.NODE_ENV !== 'production') {
MarkdownDocs.propTypes = exactProp(MarkdownDocs.propTypes);
}

export default withStyles(styles)(MarkdownDocs);
18 changes: 6 additions & 12 deletions docs/src/modules/components/TopLayoutBlog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import Link from '@material-ui/core/Link';
import useMarkdownDocs from 'docs/src/modules/components/useMarkdownDocs';
import { getHeaders, getTitle, getDescription } from 'docs/src/modules/utils/parseMarkdown';
import AppFooter from 'docs/src/modules/components/AppFooter';
import { exactProp } from '@material-ui/utils';

const styles = (theme) => ({
root: {
Expand Down Expand Up @@ -44,18 +45,10 @@ const styles = (theme) => ({
});

function TopLayoutBlog(props) {
const {
classes,
markdown: markdownProp,
markdownLocation: markdownLocationProp,
req,
reqPrefix,
reqSource,
} = props;
const { classes, markdown: markdownProp, req, reqPrefix, reqSource } = props;

const markdownDocs = useMarkdownDocs({
markdown: markdownProp,
markdownLocation: markdownLocationProp,
req,
reqPrefix,
reqSource,
Expand Down Expand Up @@ -90,12 +83,13 @@ function TopLayoutBlog(props) {
TopLayoutBlog.propTypes = {
classes: PropTypes.object.isRequired,
markdown: PropTypes.string,
// You can define the direction location of the markdown file.
// Otherwise, we try to determine it with an heuristic.
markdownLocation: PropTypes.string,
req: PropTypes.func,
reqPrefix: PropTypes.string,
reqSource: PropTypes.func,
};

if (process.env.NODE_ENV !== 'production') {
TopLayoutBlog.propTypes = exactProp(TopLayoutBlog.propTypes);
}

export default withStyles(styles)(TopLayoutBlog);
12 changes: 3 additions & 9 deletions docs/src/modules/components/useMarkdownDocs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,7 @@ import MarkdownElement from 'docs/src/modules/components/MarkdownElement';
import { LANGUAGES_IN_PROGRESS, SOURCE_CODE_ROOT_URL } from 'docs/src/modules/constants';

export default function useMarkdownDocs(options) {
const {
markdownLocation: locationProp,
markdown: markdownProp,
req,
reqPrefix,
reqSource,
} = options;
const { markdown: markdownProp, req, reqPrefix, reqSource } = options;

const userLanguage = useSelector((state) => state.options.userLanguage);
let demos;
Expand Down Expand Up @@ -58,10 +52,10 @@ export default function useMarkdownDocs(options) {
const headers = getHeaders(markdown);

const { activePage } = React.useContext(PageContext);
let location = locationProp || (activePage && activePage.pathname);
let location = activePage && activePage.pathname;
const t = useSelector((state) => state.options.t);

if (location && !locationProp) {
if (location) {
const token = location.split('/');
token.push(token[token.length - 1]);
location = token.join('/');
Expand Down
21 changes: 2 additions & 19 deletions docs/src/modules/redux/initRedux.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable no-underscore-dangle */
import { createStore, combineReducers, applyMiddleware, compose } from 'redux';
import optionsReducer from 'docs/src/modules/redux/optionsReducer';
import { createLogger } from 'redux-logger';

// Get the Redux DevTools extension and fallback to a no-op function
let devtools = (x) => x;
Expand All @@ -13,7 +14,7 @@ if (
devtools = window.__REDUX_DEVTOOLS_EXTENSION__();
}

function create(initialState) {
export default function create(initialState) {
let middleware = [];

if (
Expand All @@ -23,9 +24,6 @@ function create(initialState) {
// redux-logger needs this feature
Object.hasOwnProperty('assign')
) {
// eslint-disable-next-line global-require
const createLogger = require('redux-logger').createLogger;
Copy link
Member Author

@eps1lon eps1lon Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Webpack is clever enough to remove redux-logger from prod bundles (at least the bundle size didn't show that so it's either bundled anyway or working as expected). One less global require :)


middleware = [...middleware, createLogger()];
}

Expand All @@ -37,18 +35,3 @@ function create(initialState) {
compose(applyMiddleware(...middleware), devtools),
);
}

export default function initRedux(initialState) {
// Make sure to create a new store for every server-side request so that data
// isn't shared between connections (which would be bad)
if (!process.browser) {
return create(initialState);
}

// Reuse store on the client-side
if (!global.__INIT_REDUX_STORE__) {
Copy link
Member

@oliviertassinari oliviertassinari Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Note for context.

The incentive for the global was to account for cases where Next.js doesn't bundle redux into the share JavaScript bundle. In the past, I had multiple cases where we would get a new instance of redux per page. I don't know if it's still a concern today.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would we get multiple instances? There's only a single store.

Copy link
Member

@oliviertassinari oliviertassinari Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Basically, Next.js/webpack would fail to identify Redux as a module that needs to be put into the shared bundle. In some cases, we had:

  • shared_bundle.js
  • page_a.js contains docs/src/modules/redux/initRedux.js
  • page_b.js contains docs/src/modules/redux/initRedux.js (duplicate)

the JavaScript reference doesn't match, we end-up with two Redux store loading when transitioning from page a to page b, page b has no prior knowledge of the store of page a.

I imagine it can be reproduced in dev mode, where we only build one page at the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if this were the case: What is the problem? These are pure functions (ignoring devtools). We can call them how often we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

And again: Every page could have its own module. We would still use the same store across pages since AppWrapper stays mounted.

Copy link
Member

@oliviertassinari oliviertassinari Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It was causing an issue with the states that aren't persisted. For instance, if we store the dark theme into redux or the color of the page, we would lose it with the first page transition.

As long as we don't notice a behavior in this order, I think it's safe to move forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

To illustrate: You're seeing the flamegraph of the commit after we navigated from / to /components/box. "ne" is the AppWrapper component that got updated. It didn't "render for the first time" i.e. mount. What mounted was the page (the big yellow bar):

Screenshot from 2020-04-16 21-51-07

The store wasn't re-created. That only happens when AppWrapper mounts which happens once.

Copy link
Member

Choose a reason for hiding this comment

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

👌

global.__INIT_REDUX_STORE__ = create(initialState);
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
}

return global.__INIT_REDUX_STORE__;
}