Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

feat(@schematics/angular): Add directive and component selector rules #644

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

chrste90
Copy link
Contributor

@chrste90 chrste90 commented Apr 6, 2018

Here is the initial implementation and fix of #570

Some questions:

  1. When i add the schematics/angular/application/files/root/tslint.json, the workspace tslint file is not copied when i run ng new App, only the extending tslint exists in the created application. Is it not possible to create two files with the same name in different directories or am i missing something?
  2. The LibraryOptions don't have a property for prefix, so i just chose lib for now. Should i add a prefix property and use it?
  3. Should i add tests somewhere?

CC @filipesilva

@filipesilva
Copy link
Contributor

@Brocco could you have a look please? Mike really is the authority on these things so I'd defer to him.

@Brocco
Copy link
Contributor

Brocco commented Apr 6, 2018

Answers to your questions

  1. No issues with the same file name in multiple dirs... root for new applications is src, check there
  2. Yes, library should have a prefix option defined.
  3. Absolutely, tests for the application schematic are here.

Nice work thus far!

@chrste90
Copy link
Contributor Author

chrste90 commented Apr 7, 2018

@Brocco I made all the changes.

Only the problem 1 persists and i don't really know what i can do.

As soon as i add schematics/angular/application/files/root/tslint.json, the schematics/angular/workspace/files/tslint.json isn't copied any more to a new angular project. If i rename the schematics/angular/application/files/root/tslint.json to tslint2.json, the workspace file is copied again. Any idea what the problem could be?

@filipesilva
Copy link
Contributor

@chrste90 now that you mention it again, I think I ran into a similar problem in the library schematics...
https://github.com/angular/devkit/blob/master/packages/schematics/angular/library/index.ts#L181-L190

Having moved files with the same name should work, but is bugged and does not. The workaround is to have a template folder named __something__ with the problematic file there, passing the something var to the schematics. Schematics will replace that folder name with the path in the variable.

When I did that I removed the move in favor of using the template folder but I think it is more correct to leave it in, and only add the template folder for the problematic file.

@chrste90
Copy link
Contributor Author

chrste90 commented Apr 7, 2018

Thanks @filipesilva
I made the changes and it seems that everything is working now.
So i think the changes are ready to review.

@chrste90
Copy link
Contributor Author

chrste90 commented Apr 7, 2018

Also another thing (maybe it's a known problem):
When i run the tests locally on my macbook, one test fails:

1) NodeJsSyncHost can watch
  Message:
    Expected 0 to be 3.
  Stack:
    Error: Expected 0 to be 3.
        at Promise.resolve.then.then.then.then.then.then.then.then (/Users/chris/Desktop/devkit/packages/angular_devkit/core/node/file:/Users/chris/Desktop/devkit/packages/angular_devkit/core/node/host_spec.ts:101:34)
        at <anonymous>

@chrste90 chrste90 force-pushed the add-tslint-rules branch 2 times, most recently from 542e93a to f33405b Compare April 7, 2018 11:05
@filipesilva filipesilva requested a review from Brocco April 10, 2018 14:35
@filipesilva
Copy link
Contributor

@chrste90 yeah that test is flaky on some environments. Not too sure why myself, but it is known.

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, thanks!

@Brocco Brocco merged commit 6dbcd78 into angular:master Apr 12, 2018
@chrste90 chrste90 deleted the add-tslint-rules branch April 12, 2018 09:09
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.

4 participants