fix(schematics): avoid lastIndexOf error while creating store schematics (#1659)

[EnricoVogt]

fix(schematics): avoid lastIndexOf error while creating store schematics (#1659)

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #1659

What is the new behavior?

Now its possible to create a store schematic without a name if its called with the "--root" flag, otherwise a name is necessary.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[ngrxbot]

Preview docs changes for 3d06428 at https://previews.ngrx.io/pr1666-3d06428/

[King-i]

My 2 cents is if name is a required parameter then it may be better for it to be specified in schema.json under required which will prevent a user being able to run the schematic as stated in the schematics specifications, checking if the value was provided but having it declared as a required type in schema.ts defeats the purpose of the type system and the configuration via schema, if unit tests are required to confirm that schema.json matches schema.ts then that can be done via a AST read and compare?

[EnricoVogt]

I think the name parameter should not be required. If you create a rootstate (--root) the name is definitely not used. For my part i wont write something that is not necessary.

I have an open mind for other solutions.
I will wait for a advice how to solve it.

@brandonroberts
@timdeschryver

[King-i]

If that's the case then no it shouldn't be set as required, however the definition says its required, so therefore the schema interface needs to be updated to have name set as optional, which may have caught this bug in the original unit tests in the first place. Also parseName is a utility function used in multiple projects, changing the signature of this function to accept an empty string as default could introduce unintended consequences elsewhere?

[ngrxbot]

Preview docs changes for 1f251ad at https://previews.ngrx.io/pr1666-1f251ad/

[timdeschryver]

Suggested change

	expect(function() {
	expect(() => {

[EnricoVogt]

Done.

[timdeschryver]

Suggested change

	expect(thrownError).toBeDefined();
	expect(() => {
	schematicRunner.runSchematic('store', options, appTree);
	}).toThrowError('Please provide a name for the feature state');

Also, remove the try catch

[EnricoVogt]

Did this for my test now. There is another test "should fail if specified module does not exist" which use also the try catch - should i change this too?

[timdeschryver]

Not in the PR, you can open up a new PR if you would like to

[ngrxbot]

Preview docs changes for 1a70cc3 at https://previews.ngrx.io/pr1666-1a70cc3/

[timdeschryver]

LGTM 👍