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

feat(@schematics/angular): strict option #705

Closed
wants to merge 1 commit into from
Closed

feat(@schematics/angular): strict option #705

wants to merge 1 commit into from

Conversation

cyrilletuzi
Copy link
Contributor

@cyrilletuzi cyrilletuzi commented Apr 14, 2018

--strict option to configure TypeScript in strict mode when generating an app.

Replaces #397

Redo of #462 because of merge issues.

Updated with last schematics, and there is no side effect, so if you want to merge for v6, that would be great.

Note this PR is for v6 only, as strictFunctionTypes requires TS 2.6.

@hansl @filipesilva @Brocco

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.

Missing: the logic to pass the option from new to workspace

const workspaceOptions: WorkspaceOptions = {

Also, please include tests to verify that the value is set appropriately.

Another note, please also update the documentation to include the option in the CLI repo: https://github.com./angular/angular-cli/blob/master/docs/documentation/new.md

@cyrilletuzi
Copy link
Contributor Author

@Brocco Logic and test added, doc added in angular/angular-cli#10497

@cyrilletuzi
Copy link
Contributor Author

AppVeyor failure not due to this PR.

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.

One comment for the review.

This is a feature and will not be part of 6.0.0, please create an issue associated for this PR so it can be included and tracked for the 6.1.0 release.

@@ -46,4 +46,18 @@ describe('Ng New Schematic', () => {
const content = tree.readContent('/bar/angular.json');
expect(content).toMatch(/"prefix": "pre"/);
});

it('should set strict options in tsconfig.json', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should live in the workspace schematic, because that is where the logic exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? I put it there on purpose, because even if the logic is in the workspace, the user will use this option from ng new, so putting it there also verify that the option is passed well (to avoid problems like in my initial PR where I missed the passing logic). Or maybe do the test twice?

@cyrilletuzi
Copy link
Contributor Author

cyrilletuzi commented May 11, 2018

@hansl @Brocco @StephenFluin Dropped my other features PR, as I've find other ways, but this one would be helpful (as we can't just set strict: true as property initialization check does not work well with Angular, so there is currently 5 options to manually change, and there will be more and more in the future).

In the end it's your decision, so let me know if I should close this PR or not.

@alexeagle
Copy link
Contributor

Sorry @cyrilletuzi - this got lost when we moved back to the angular/angular-cli repo. If you still think this pull request is relevant, could you please re-base on that repo and open a new PR there? Thanks and sorry for the extra churn.

@alexeagle alexeagle closed this Sep 5, 2018
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