feat(@angular-devkit/schematics): change schema validation to AJV #334
Conversation
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
filipesilva
changed the title
| @@ -105,6 +105,13 @@ export interface Tree { | |||
| } | |||
|
|
|||
|
|
|||
| namespace Tree { | |||
| export function isTree(maybeTree: object): maybeTree is Tree { | |||
There was a problem hiding this comment.
This doesn't seem to be used.
There was a problem hiding this comment.
I think it should be where useful (instead of using TreeSymbol in Tree). It also communicates the types better to TypeScript. It doens't have to be part of this PR.
| .subscribe(done, done.fail); | ||
| }); | ||
|
|
||
| it('works synchronously', done => { |
There was a problem hiding this comment.
There should be a way to fail synchronously if it's not sync. Otherwise you can't really know if it is sync or async and need to always prepare for it to be async.
There was a problem hiding this comment.
You can add the check, but this is used internally by ourselves so I don't see the need. We can always use the Observable as an Observable.
There was a problem hiding this comment.
Added a comment with this information.
| @@ -13,3 +13,6 @@ export { FallbackEngineHost } from './fallback-engine-host'; | |||
| export { FileSystemEngineHost } from './file-system-engine-host'; | |||
| export { NodeModulesEngineHost } from './node-module-engine-host'; | |||
| export { NodeModulesTestEngineHost } from './node-modules-test-engine-host'; | |||
|
|
|||
| export { AjvSchemaRegistry } from './ajv-option-transform'; | |||
There was a problem hiding this comment.
I think we're going to have to move this to core if we want to share code with Build Facade or other things.
scripts/test.ts
Outdated
| @@ -25,6 +25,11 @@ require('source-map-support').install({ | |||
| }); | |||
|
|
|||
|
|
|||
| interface CoverageLocation { | |||
There was a problem hiding this comment.
This should be part of the test: add an istanbul bootstrap script commit.
There was a problem hiding this comment.
It should. I was just pushing stuff as quickly as I could :( sorry. I normally clean up before submitting for review.
There was a problem hiding this comment.
I was leaving the comment here mostly for myself when reworking this PR. I'll move it to the other commit.
| name: 'path', | ||
| formatter: { | ||
| async: false, | ||
| validate: (path: string) => { |
There was a problem hiding this comment.
We should normalize the path instead of verifying if it's normalized already.
There was a problem hiding this comment.
We can't do that in a format though. It just verifies that the data is following a format, it can't modify it.
When the test script is loaded it is already too late, and we are missing a lot of information for coverage. This makes sure we get coverage for everything (except bootstrap itself).
| import { htmlSelectorRe } from './html-selector'; | ||
|
|
||
|
|
||
| const unsupportedProjectNames = ['test', 'ember', 'ember-cli', 'vendor', 'app']; |
There was a problem hiding this comment.
Should the ember references be removed? The existing CLI logic has them but that was really more of a carryover from the original ember code.
|
CLA: All authors of this PR have signed the CLA, so I'm overriding it. |
Followup from #280