Skip to content
This repository has been archived by the owner on Apr 9, 2022. It is now read-only.

json-schema yak shaving for schematics #163

Merged
merged 33 commits into from Sep 26, 2017
Merged

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Sep 21, 2017

Proper description for each commits.

@@ -112,21 +113,40 @@ export function resolve(x: string, options: ResolveOptions = {}): string {
}

// Fallback to checking the global node modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment mismatch, should be on global check below.

return super._resolveCollectionPath(name);
}

// protected abstract _transformCollectionDescription(
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments can be discarded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All done and rebased on top of master.

@Splaktar
Copy link
Member

Splaktar commented Sep 26, 2017

There isn't much of a description for the overall changes behind this large PR, but Hans mentioned some of it here.

Some key details from that:

It will resolve using the following list:

is it a node package relative to process.cwd()?
is it a node package relative to the tool (CLI in your case)?
is it a node package installed globally?

You might notice there is two missing fallbacks that's missing; is it a package relative to your schematics and is it a package relative to your project? That's coming, it's just a bit more complicated to implement. In any case, you could install your schematic globally and that will work fine.

I'm glad to hear that this is coming. I guess that it makes sense to handle the global case first since that is needed for creating a new application.

I'm working with an enterprise client atm and we need a solution for installing our schematics into the given project along with the @angular-cli under devDepenedencies. Currently we install them in an npm package like node_modules/my-schematics/ in our project (via a devDependency) and would like to be able to then generate our custom components/services.

I was wondering if it was safe for me to work with schematics on the 1.4.3 CLI builds (where I started), but with critical pieces like this still coming in, it looks like I should move all of our efforts over to 1.5.0-beta?

@hansl
Copy link
Contributor Author

hansl commented Sep 26, 2017

@Splaktar There isn't much of a description for the PR as it is really just a list of commits. Each commit is self explanatory (and since we use the git history as documentation, the PR description is basically relevant only for review context).

I was wondering if it was safe for me to work with schematics on the 1.4.3 CLI builds (where I started), but with critical pieces like this still coming in, it looks like I should move all of our efforts over to 1.5.0-beta?

If I were you I would not rely on CLI itself for Schematics development. We're coming up with a tutorial, but you should try to use @angular-devkit/schematics directly (there is a command line tool for schematics that will include more debug information). In the general case there is no differences between running schematics and ng generate (differences are handling of the angular-cli config which we are in the process of rewriting, so there'll be even less differences in the future).

package-lock.json was out of date.
This used to be only used by schematics, but the Core might need it
for using templates internally.
Those files are using template() to be built, and will be built in place and
released on npm. You can require those templates if you need, but they need
to have a .ejs extension.

This required embedding the escape function so that templates' source
can be executed without any arguments. Additionally, also added default
options to the template function.
Previously only spec files could do it. Benchmark files are similar
enough to specs that they should be allowed to disable tslint globally.
The only one thats supported right now is stripIndents. This is in
order to remove the common-tags dependency from the CLI.
This is exported a "schema" from core.

This replaces the @ngtools/json-schema library from before. Benchmarks show
a 4x improvements on access, at the cost of having to eval a lot of code
when parsing the schema, in order to build the proxies and validators for
values.

This has support for future improvements such as subschema and schema
references, which were not possible with the old library. Additionally, the
hit for parsing can be reduced significantly later.

For now, compatibility with the CLI configuration was the main goal of this
feature. Parity with the old system has been achieved (except for .d.ts
serialization which is coming next), and new features have been added.
In the default schematics CLI we now use the core json-schema library.
Options that have invalid type, or are not defined, or are required will
now output a valid error message that is closer to the actual error.

The next step is to properly log exceptions to the user so theyll know
what action to take.
Passing -- on the command line will continue parsing arguments after
the --, but does not distinguish with arguments that are supported
by schematics directly.

e.g.  "schematics test --arg1=abc -- --dry-run=hello"
will have two arguments [ args, dry-run ]. If the -- was not passed
in dry-run would be removed from the options to the schematics.
Moving this in the observable has the nice property that errors are
propagated through the observable, and not thrown up in the call().
Node 6 does not support "..." which was being used in our templates.
This removes the need to support it.
Add a better description when the error is "RequiredValueMissing".
Add a better description when the error is "InvalidPropertyNameException".
This is inspired by the resolve package on npm, but with a few modifications:

- it is in typescript
- it has less options and is more straightforward
- it can fallback to checking the global node registry if it doesnt find the package

Also added the package angular_devkit/core/node since those utilities
are node specific, but the code tries to be platform agnostic.
This adds a few things:

- NodeModulesTestEngineHost. This uses the same logic as NodeModulesEngineHost,
  but allow for overwriting some scheamtics with specific paths. This prevents
  the EngineHost from using node_modules, which doesnt contain the collection
  we are trying to test.
- Remove RegistryEngineHost which is not used.
- Add a new FallbackEngineHost, which can resolve collections and schematics
  against multiple hosts as a fallback pattenr.
- Use the new NodeModulesTestEngineHost for SchematicsTestRunner.
- Update all the tests to use the new stuff.
- When resolving Url to Sources, knowing the context when creating the source
  is necessary. Changed that.
- The NodeModulesEngineHost now uses the resolve() method from
  @angular-devkit/core/node.
Turns out the regex is really slow because it makes a lot of captures. This
new one is instant.
… path

Collections can now understand relative paths to a package.json or paths
to a directory that contains a package.json.

Also added the capacity for the "schematics" key in package.json to
point to a node module which will be resolved relative to the
package.json containing the key.
Prior to this, contexts were created inline where they were needed. Adding
the creation in the Engine will allow us to refer to the host when creating
a context. Also added the ability to pass in a parent context for transfering
information (such as merge strategy).
This will allow us to add more details (and log events) related to schematics.
Core is getting crowded, so more stuff will be put in its own namespace.
Also remove the usage of common-tags everywhere.
Using the source-map library from mozilla. Its pretty neat.
Before it was a full Logger, now we properly export a new LoggerApi
that only contain logging APIs. We also added a facade to avoid
recasting shenanigans.
This simplifies writing simple rules that only change the tree.
So that the collection.json can include comments and other goodies. Note that the
package.json is still using require() as it is a regular node file and would fail
npm anyway, so no incentive for us to make it loose.
Before we were messing with ownKeys to only return defined properties, but that
has the side effect that those will not work inside a "with() {}" clause, which
is what templates are using.

We now need a serialize symbol if we want to only serialize defined properties,
but for now parity with @ngtools/json-schema is not a priority.
Copy link
Contributor

@Brocco Brocco left a comment

Choose a reason for hiding this comment

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

LGTM

@hansl hansl merged commit 26f78ab into angular:master Sep 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants