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

Upgraded Deps and NGRX 4.x.x #7

Closed
wants to merge 6 commits into from
Closed

Upgraded Deps and NGRX 4.x.x #7

wants to merge 6 commits into from

Conversation

Kaffiend
Copy link

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Updates Angular, NGRX, and Typescript with current versions.

  • What is the new behavior (if this is a feature change)?

  • Other information:
    In app.module.ts HMR init function used to stringify state and log it to the console. This needs to be revisited if its really desired. A replacer or serializer will need to be used, @ngrx/router-store includes routerState which contains circular structure.

@Kaffiend
Copy link
Author

Kaffiend commented Jul 29, 2017

completes #6

@Kaffiend
Copy link
Author

this may resolve #5 as well...

@Kaffiend
Copy link
Author

need to update the tests.

@Kaffiend
Copy link
Author

fixing AOT

@Kaffiend
Copy link
Author

@colinskow looks like everything else is fine, just AOT needs some love. I'll take another look at this tomorrow. unless someone else can figure it out before hand.

@Kaffiend
Copy link
Author

currently ngrx-store-freeze is not compatible with v4, see here so I removed it, until it can be updated. I suspect this PR will remain open and we can update it as NGRX and dependent libs progress.

@colinskow
Copy link
Owner

Thank you very much! I will have a chance to review next Tues or Weds.

@colinskow
Copy link
Owner

How is the progress on this going? It looks like AngularClass now has a new HMR loader that works out of the box with ngrx 4.

@Kaffiend are you interested in finishing this, or should I go ahead. It looks fairly simple.

@Css-IanM
Copy link

Css-IanM commented Aug 31, 2017

I have been swamped, of late. I might be able to get to this, this weekend. Unless you can find time before then. I dont mind either way. I havent seen the new loader as i've been busy on some work projects. Ill have to check that out. This PR works with everything but AOT i think, if i recall correctly. But would need updated with the new loader it sounds like.

Derp: was still on my work account.

@Kaffiend
Copy link
Author

Kaffiend commented Aug 31, 2017 via email

@colinskow
Copy link
Owner

The main issue is that the build needs to pass, including with AoT. It looks like ngrx-store-freeze is still not ready for 4.0. Perhaps we can stub the code in but comment it out until ready.

Over the next few days I'll take a look at your PR and compare it to the official migration guide to ensure everything is standard. We can merge when the build is passing.

@Css-IanM
Copy link

Css-IanM commented Sep 2, 2017 via email

@Kaffiend
Copy link
Author

Kaffiend commented Sep 2, 2017

i'll try to get to this either later this evening or tomorrow morning, Had some time sensitive issues crop up in some old COBOL apps that requires my attention for the better part of this weekend unfortunately.

@colinskow
Copy link
Owner

@Kaffiend thank you very much for your work on this. Using your changes as a guide I was able to work things out and get the build passing, even with store-freeze working. However I did a slightly different implementation so it was easier for me to copy and paste from your code than to try to rebase your fork.

I will be merging this feature in very shortly after some final polish.

@Css-IanM
Copy link

Css-IanM commented Sep 25, 2017 via email

@colinskow colinskow closed this in 83646a9 Sep 25, 2017
@colinskow
Copy link
Owner

DONE!

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.

None yet

3 participants