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(hmr-plugin): correct persistence state in runtime #1048

Merged
merged 18 commits into from
May 17, 2019
Merged

Conversation

splincode
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

#917

What is the new behavior?

  • Current HMR-plugin worked incorrectly, now this saving works through a global object and and is used before other plugins are initialized and states bootstraped.

  • Correct Life-cycle events triggered in root state and others

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@splincode
Copy link
Member Author

@markwhitfeld please take a look

@splincode splincode added this to the 3.5.0 milestone May 13, 2019
Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

Which test does this PR fix?
I don't like the HMR stuff in the store internals and I think I can make some small modifications that would fix this but I need to know which test this work aims to fix.

@splincode
Copy link
Member Author

@markwhitfeld

I don't like the HMR stuff in the store internals

I understand, but the correct work of the HMR

  • is to write the current state before destroyed module into some variable (in global window)
  • and initialize it when initializing the behaviorSubject in stateFactory

@markwhitfeld
Copy link
Member

Ok. That helps me understand your reasoning. I think that I can clean this up so that we don't need to have HMR in the internals. Let me try. I'll commit to this branch

@splincode
Copy link
Member Author

@markwhitfeld Thank you!

@splincode
Copy link
Member Author

@markwhitfeld we need remove defaultState parameter, because it turned out to be useless

@markwhitfeld markwhitfeld requested a review from arturovt May 15, 2019 22:28
@markwhitfeld
Copy link
Member

@splincode I agree. We need to add the removal of defaultState to the list for v4.

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

I'm happy with this now.
@arturovt could you review?
@splincode could you test to make sure that the issue is fixed?

@markwhitfeld
Copy link
Member

One step forward two steps back! The nuances of hmr are killing me.
Hopefully all sorted out now.

@markwhitfeld markwhitfeld requested a review from arturovt May 16, 2019 08:25
@splincode
Copy link
Member Author

@markwhitfeld LGTM

@splincode splincode requested a review from markwhitfeld May 16, 2019 08:25
@markwhitfeld markwhitfeld self-assigned this May 16, 2019
Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

I'm good with this.
@arturovt could you review and approve for merging in.

Copy link
Member

@arturovt arturovt left a comment

Choose a reason for hiding this comment

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

LGTM

@splincode
Copy link
Member Author

@markwhitfeld @arturovt I will see later does it correct work

@markwhitfeld
Copy link
Member

@splincode great, then you are welcome to merge if it does.

@splincode
Copy link
Member Author

@markwhitfeld everything works perfect

@splincode splincode merged commit fc264ae into master May 17, 2019
@splincode splincode deleted the fix/lifecycle branch June 30, 2019 11:52
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