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

feat: implement ng add for store and effects packages #1067

Merged
merged 16 commits into from
May 23, 2018
Merged

feat: implement ng add for store and effects packages #1067

merged 16 commits into from
May 23, 2018

Conversation

vitaliy-bobrov
Copy link
Contributor

@vitaliy-bobrov vitaliy-bobrov commented May 14, 2018

Implementation of #920 WIP.

This is the implementation of @ngrx/store ng-add module to get feedback, cause other ng-adds will be done in a similar way.

Done:

Additional code share optimizations could be performed later, after initial implementation.

Will work on other modules after gathering a feedback.

@coveralls
Copy link

coveralls commented May 14, 2018

Coverage Status

Coverage increased (+0.7%) to 87.607% when pulling 26a065f on vitaliy-bobrov:ng-add into 440b6cf on ngrx:master.

# Conflicts:
#	modules/effects/schematics-core/index.ts
#	modules/effects/schematics-core/templates/store/__path__/__statePath__/index.ts
#	modules/entity/schematics-core/index.ts
#	modules/router-store/schematics-core/index.tsa
#	modules/schematics-core/index.ts
#	modules/schematics/schematics-core/index.ts
#	modules/schematics/src/store/files/__path__/__statePath__/index.ts
#	modules/schematics/src/store/files/__statePath__/index.ts
#	modules/store-devtools/schematics-core/index.ts
#	modules/store/schematics-core/index.ts
@@ -0,0 +1,18 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Remove this file. The npm_package already has a default tsconfig.json` to use

addPackageToPackageJson,
platformVersion,
parseName,
} from '../../schematics-core';
Copy link
Member

Choose a reason for hiding this comment

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

Use the @ngrx/schematics/schematics-core import here

appTree = createWorkspace(schematicRunner, appTree);
});

it('should update package.json', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Add a test for the skipPackageJson option

@@ -19,8 +22,11 @@
"homepage": "https://github.com/ngrx/platform#readme",
"peerDependencies": {
"@angular/core": "NG_VERSION",
"@angular-devkit/core": "NG_DEVKIT_VERSION",
Copy link
Member

Choose a reason for hiding this comment

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

Remove these from the package.json as we are treating them as implicit dependencies instead of adding additional dependencies to @ngrx/store

.prettierignore Outdated
@@ -1,5 +1,7 @@
/dist
/modules/schematics/src/*/files/*
/modules/schematics-core/templates/*
Copy link
Member

Choose a reason for hiding this comment

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

Change templates to files so its consistent with schematics. If we want to change files to templates across the board, we'll do that in a separate PR.

@@ -0,0 +1,34 @@
package(default_visibility = ["//visibility:public"])
Copy link
Member

Choose a reason for hiding this comment

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

This should be added to the packages section in the modules/store/BUILD file in the npm_package. That way its output gets copied into the distribution.

@brandonroberts
Copy link
Member

brandonroberts commented May 15, 2018

I left some feedback. I'd rather have 2 copies of the store schematics template files, then put them into the schematics-core and have 6 copies in places where they will never be used.

@vitaliy-bobrov
Copy link
Contributor Author

Thanks for the review, will work on changes. Regarding templates, I also not sure about keeping them in core, just thought that it could be good to have the only source.

@brandonroberts
Copy link
Member

Since its only one file, I think it will be ok to have 2 copies.

const relativePath = buildRelativePath(modulePath, statePath);
const environmentsPath = buildRelativePath(
statePath,
`/${options.path}/environments/environment`
Copy link
Member

Choose a reason for hiding this comment

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

addPackageToPackageJson,
platformVersion,
parseName,
} from '@ngrx/schematics/schematics-core';
Copy link
Member

Choose a reason for hiding this comment

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

This should be @ngrx/store/schematics-core

@@ -21,6 +24,7 @@
"@angular/core": "NG_VERSION",
"rxjs": "RXJS_VERSION"
},
"schematics": "./schematics/collection.json",
Copy link
Member

Choose a reason for hiding this comment

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

Add this replacement to /tools/default.bzl

@brandonroberts
Copy link
Member

Added a couple more changes, otherwise I think this is ready to implement in the other packages.

@brandonroberts brandonroberts changed the title Implement ng add (WIP) feat: Implement ng add for store and effects packages May 23, 2018
@brandonroberts brandonroberts changed the title feat: Implement ng add for store and effects packages feat: implement ng add for store and effects packages May 23, 2018
@brandonroberts brandonroberts merged commit db94db7 into ngrx:master May 23, 2018
@vitaliy-bobrov vitaliy-bobrov deleted the ng-add branch May 24, 2018 09:21
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