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

fix(v2): fix hot reload sometimes not working due to altered modules #1370

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

endiliey
Copy link
Contributor

Motivation

Sometimes hot reload not working because we alter the modules object.

Try to deep clone the object with json clone trick. Lodash cloneDeep would do the job but lodash cloneDeep can make our bundle size bigger :| and according to lodash/lodash#1984 (comment) it is also slower than simple JSON clone trick.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

yarn start --hot-only

Hot reload working
image

Related PRs

#1366 #1367

@endiliey endiliey requested a review from yangshun as a code owner April 18, 2019 11:00
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 18, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit 2427265

https://deploy-preview-1370--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit 2427265

https://deploy-preview-1370--docusaurus-preview.netlify.com

@endiliey endiliey merged commit b4daac9 into facebook:master Apr 18, 2019
@endiliey endiliey deleted the hot branch April 18, 2019 11:04
// Transform back loaded modules back into the original structure.
const loadedModules = originalModules;
// clone the original object since we don't want to alter the original.
const loadedModules = JSON.parse(JSON.stringify(modules));
Copy link
Contributor

@yangshun yangshun Apr 18, 2019

Choose a reason for hiding this comment

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

Interesting. I thought I was using _.cloneDeep here. If we import just the function it's fine:

import cloneDeep from 'lodash/cloneDeep`; // It's ok, just cloneDeep is imported
// vs
import _ from 'lodash`; // Not ok, the entire lodash is imported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sry I removed it before because bundle size is still very big. Few KBs saved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thanks for fixing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants