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

Added support to give name for store dev tool #517

Merged
merged 9 commits into from
Nov 13, 2017

Conversation

rupeshtiwari
Copy link
Contributor

This is the changes to put name on store dev tool configuration object for the issue #463

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 92.546% when pulling e0b05f4 on roopkt:dev-tool-name into 565389a on ngrx:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 92.546% when pulling 026d4d2 on roopkt:dev-tool-name into 565389a on ngrx:master.

@rupeshtiwari
Copy link
Contributor Author

Finished passing name to devtools fixes #463

@rupeshtiwari
Copy link
Contributor Author

Hi @brandonroberts I updated this pull request with my latest changes.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 92.534% when pulling 4e8b793 on roopkt:dev-tool-name into 565389a on ngrx:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 92.488% when pulling 7a891a7 on roopkt:dev-tool-name into 565389a on ngrx:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 92.488% when pulling 0bb84da on roopkt:dev-tool-name into ab7de5c on ngrx:master.

@rupeshtiwari
Copy link
Contributor Author

Hi @brandonroberts ,
I fixed code merge issue as well. Please check if the implementation or the fix for the issue #463 is correct.

@brandonroberts
Copy link
Member

@roopkt the changelog shouldn't be included in your changes. Please verify that your changes change the name in the extension also.

@rupeshtiwari
Copy link
Contributor Author

rupeshtiwari commented Nov 2, 2017

Hi @brandonroberts thanks for giving me the direction to pass options to send method. I am planning to inject then STORE_DEVTOOLS_CONFIG into DevtoolsExtension so that we can always pass the entire option in send method. This is one potential solution that I think of.

I even wanted to validate by running example app however unfortunately when I run
yarn run eample:star I always get error NONEMPTY dir related error. I tried deleting '../platform/dist' folder manually still when I run the start script I get error at some point while "Copying type definition files" step. I event tried opening new command terminal to execute still same issue. Have you encountered this issue is there any wise workaround of this.

Example of error:

× Copying type definition files
{ Error: ENOTEMPTY: directory not empty, rmdir '...\platform\dist\packages\enti
ty'
  errno: -4051,
  code: 'ENOTEMPTY',
  syscall: 'rmdir',
  path: '...\\platform\\dist\\packages\\entity' }
error Command failed with exit code 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

This error is big problem and wasting lots of time. I want to know the process of testing changes. When I do some changes in any module in ngrx then for testing the same in example app what is my step ? I tried below close vscode, delete platform/dist folder open vscode and run yarn run example:start but it shows above error.

…ts to verify its really ending options to redux dev tool extension ngrx#517
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 92.813% when pulling fe9d0bb on roopkt:dev-tool-name into ab7de5c on ngrx:master.

@rupeshtiwari
Copy link
Contributor Author

Hey @brandonroberts
I pushed one more change-set this time I verified the ReduxDevToolExtension name is indeed changing. 👍

CHANGELOG.md Outdated
@@ -1,6 +1,204 @@
# Changelogs
Copy link
Member

Choose a reason for hiding this comment

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

This file should not be included in the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@@ -64,12 +64,16 @@ export function noMonitor(): null {
return null;
}

export const DEFAULT_NAME = 'ngrx-store-devtool-instance';
Copy link
Member

Choose a reason for hiding this comment

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

Change to NgRx Store DevTools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the name to NgRx Store DevTools

export function createConfig(
_options: StoreDevtoolsOptions
): StoreDevtoolsConfig {
const DEFAULT_OPTIONS: StoreDevtoolsConfig = {
const DEFAULT_OPTIONS: Partial<StoreDevtoolsConfig> = {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a partial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the partial from here.

CHANGELOG.md Outdated
@@ -1,6 +1,204 @@
# Changelogs
Copy link
Member

Choose a reason for hiding this comment

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

The changelog should not be included in these changes. We'll generate a changelog in the next release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for explaination

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 92.813% when pulling 83088f2 on roopkt:dev-tool-name into ab7de5c on ngrx:master.

@brandonroberts brandonroberts merged commit 00be3d1 into ngrx:master Nov 13, 2017
@brandonroberts
Copy link
Member

Thanks!

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