[docs] Remove old workarounds #20587
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Details of bundle changes.Comparing: fe687bf...b83ebae Details of page changes
|
oliviertassinari
approved these changes
Apr 16, 2020
|
Having a quick look at the JavaScript runtime, I would expect at least a 10% reduction of the runtime by sharing the HTML output of prism between the client and server side :) |
eps1lon
commented
Apr 16, 2020
| } | ||
|
|
||
| // Reuse store on the client-side | ||
| if (!global.__INIT_REDUX_STORE__) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
How would we get multiple instances? There's only a single store.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Even if this were the case: What is the problem? These are pure functions (ignoring devtools). We can call them how often we want.
There was a problem hiding this comment.
And again: Every page could have its own module. We would still use the same store across pages since AppWrapper stays mounted.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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):
The store wasn't re-created. That only happens when AppWrapper mounts which happens once.
eps1lon
force-pushed
the
docs/markdown-build-time-refactor
branch
from
b858c59
to
e262548
Compare
eps1lon
commented
Apr 16, 2020
| @@ -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; | |||
There was a problem hiding this comment.
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 :)
eps1lon
changed the title
|
Should get this in regardless of the "markdown at build time" effort in case "markdown at build time" needs to be reverted later. |
oliviertassinari
approved these changes
Apr 16, 2020
eps1lon
commented
Apr 16, 2020
eps1lon
commented
Apr 17, 2020
|
Nice, let's try it out :) |
This was referenced Apr 20, 2020
This was referenced Apr 29, 2020
This was referenced Apr 29, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes some older workarounds that are no longer needed and make the code needlessly complicate in their current state.
markdownLocationprop (it seems to be unused)