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

Add development mode with freeze #409

Merged
merged 4 commits into from
Jun 2, 2018

Conversation

markwhitfeld
Copy link
Member

No description provided.

@markwhitfeld markwhitfeld requested a review from amcdnl June 2, 2018 21:10
@markwhitfeld markwhitfeld force-pushed the add-dev-mode-with-freeze branch from f19bc1b to 99acec0 Compare June 2, 2018 21:18
Copy link
Member

@amcdnl amcdnl left a comment

Choose a reason for hiding this comment

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

Few nits.

type ModuleOptions = Partial<NgxsConfig>;

export function ngxsConfigFactory(options: ModuleOptions): NgxsConfig {
const config = Object.assign(new NgxsConfig(), options);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just return directly rather than assign a variable and return that in the next line?

package.json Outdated
@@ -116,6 +116,8 @@
"zone.js": "^0.8.26"
},
"dependencies": {
"@types/deep-freeze-strict": "^1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Types should be a dev dependency.

@@ -3,14 +3,21 @@ import { Injectable } from '@angular/core';
import { StateOperations } from './internals';
import { InternalDispatcher } from './dispatcher';
import { StateStream } from './state-stream';
import { NgxsConfig } from '@ngxs/store/src/symbols';

const deepFreeze = require('deep-freeze-strict');
Copy link
Member

Choose a reason for hiding this comment

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

Why use require here? import * as deepFreeze from 'deep-freeze-strict';.

@markwhitfeld markwhitfeld force-pushed the add-dev-mode-with-freeze branch from 99acec0 to 6d41b79 Compare June 2, 2018 21:22
@@ -3,14 +3,21 @@ import { Injectable } from '@angular/core';
import { StateOperations } from './internals';
import { InternalDispatcher } from './dispatcher';
import { StateStream } from './state-stream';
import { NgxsConfig } from './symbols';

const deepFreeze = require('deep-freeze-strict');
Copy link
Member

Choose a reason for hiding this comment

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

Why use require here? I'd do: import * as deepFreeze from 'deep-freeze-strict';

Copy link
Member Author

Choose a reason for hiding this comment

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

That lib doesn't support import. Ngrx also had to import it this way.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Generally speaking when it doesn't support that you can use the wildcard import. :) - https://www.typescriptlang.org/docs/handbook/modules.html

Copy link
Member Author

Choose a reason for hiding this comment

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

ah. Let me try that

Copy link
Member Author

Choose a reason for hiding this comment

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

No luck. It says:

[ts] Module '"c:/Code/OpenSource/ngxs/node_modules/@types/deep-freeze-strict/index"' resolves to a non-module entity and cannot be imported using this construct.

Copy link
Member

Choose a reason for hiding this comment

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

Odd!

private _stateStream: StateStream,
private _dispatcher: InternalDispatcher,
private _config: NgxsConfig
) {}

public getRootStateOperations(): StateOperations<any> {
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove public from this method, its redudant.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@amcdnl
Copy link
Member

amcdnl commented Jun 2, 2018

Also, can you add a note to the docs and change log for this?

@markwhitfeld
Copy link
Member Author

@amcdnl I have addressed the issues you raised.

@amcdnl amcdnl merged commit bb08b17 into ngxs:master Jun 2, 2018
@amcdnl
Copy link
Member

amcdnl commented Jun 2, 2018

Merged! :)

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.

2 participants