Cleanup, adhere to own ruleset and easier setup #13

remyrylan · 2018-04-30T15:40:24Z

I changed around quite a bit of stuff, so there are quite of bit of changes to go through:

The current setup instructions say to add "rulesDirectory": ["node_modules/vrsource-tslint-rules/rules"] to your tsconfig.json. This is more complicated than the normal shared Tslint configs, which just use {extends: [...]}. Additionally, this approach fails if you have vrsource-tslint-rules installed further up in your project than the current directory such as monorepo structure. Using extends allows Tslint to leverage the standard node.js module resolution algorithm.
The only thing I had to do to make this work was to add "rulesDirectory": ["./rules"] to the tslint.json file in vrsource-tslint-rules. This is the same way that tslint-microsoft-contrib handles it.
Added Tslint and TypeScript as devDependencies. This is the easiest way to test peerDependencies and is recommended by plenty of projects. I use it for everything and it works great.
Make peerDependencies flexible. Npm is noisy when it comes to dependency mismatch.
Removed the generated .js files for rules that no longer have a corresponding .ts rule file. Ex: dotNotationRule.js, noDuplicateImportsRule.js, etc...
I made all the source files in vrsource-tslint-rules adhere to it's own ruleset. This included moving some bit of code around to fix no-use-before-declare warnings, and adding --project . to all of the Tslint CLI commands in order to supply Tslint with type checking information.
I regenerated all of the JS files and ran all of the tests -- everything passed for me locally.

Hope all of this works for you! Love your work, thank you for putting this out there to share.

@mycompany

The README's recommended way to use vrsource-tslint-rules's rules as part of a larger tslint.json config is to add a `rulesDirectory` entry with a relative path. However, this doesn't work if a `tslint.json` has an indirect dependency on `vrsource-tslint-rules`. For example, say my company has an `@mycompany/tslint-rules` package whose `main` file is a `tslint.json`. If `@mycompany/tslint-rules` wants to use a custom rule from `vrsource-tslint-rules`, it can try to add `"rulesDirectory": "node_modules/vrsource-tslint-rules/rules"`. However, although this path will be correct when `npm install` is run inside the directory for `@mycompany/tslin-rules`, it will be wrong when a separate `my-project` repo installs/depends on `@mycompany/tslint-rules`. In that case, the tree will be: ``` my-project - package.json - node_modules - @mycompany - tslint-rules - tslint.json - vrsource-tslint-rules - rules ``` So, the `tslint.json` file in `@mycompany/tslint-rules` would need to reference `"../../vrsource-tslint-rules/rules"` as the `rulesDirectory`. This all feels brittle (e.g., if `@mycompany/tslint-rules` was initially an unscoped package that got moved to a scope, the paths would break), and it's unnecessary. As @remyrylan notes in vrsource#13, you can (backwards- compatibly) add `"rulesDirectory"` to `vrsource-tslint-rules`'s `tslint.json`, and then consumers can import the custom rules with `"extends"`. Note: doing an `"extends"` does bring in vrsource's configuration for all the built-in tslint rules, in addition to making the custom rules available. This normally shouldn't be a problem, as consumers can easily override the configuration they don't want (including by putting the `"vrsource-tslint-rules"` earlier in the `"extends"` array than a base `"tslint"` config they're using, like `"tslint:recommended"). However, where this is a problem, we can also make it possible to truly just import the custom rules. To do that, we can add an `index.js`/ `index.ts` file to the rules directory; as the TSLint docs point out (https://palantir.github.io/tslint/usage/configuration/), having an index file leads TSLint to use node's module resolution algorithm on the `"rulesDirectory"`. This means that consumers at any level can specify `"rulesDirectory": "vrsource-tslint-rules/rules"` and have it be resolved appropriately. This commit adds that index file too, and updates the readme accordingly.

@mycompany

The README's recommended way to use vrsource-tslint-rules's rules as part of a larger tslint.json config is to add a `rulesDirectory` entry with a relative path. However, this doesn't work if a `tslint.json` has an indirect dependency on `vrsource-tslint-rules`. For example, say my company has an `@mycompany/tslint-rules` package whose `main` file is a `tslint.json`. If `@mycompany/tslint-rules` wants to use a custom rule from `vrsource-tslint-rules`, it can try to add `"rulesDirectory": "node_modules/vrsource-tslint-rules/rules"`. However, although this path will be correct when `npm install` is run inside the directory for `@mycompany/tslin-rules`, it will be wrong when a separate `my-project` repo installs/depends on `@mycompany/tslint-rules`. In that case, the tree will be: ``` my-project - package.json - node_modules - @mycompany - tslint-rules - tslint.json - vrsource-tslint-rules - rules ``` So, the `tslint.json` file in `@mycompany/tslint-rules` would need to reference `"../../vrsource-tslint-rules/rules"` as the `rulesDirectory`. This all feels brittle (e.g., if `@mycompany/tslint-rules` was initially an unscoped package that got moved to a scope, the paths would break), and it's unnecessary. As @remyrylan notes in vrsource#13, you can (backwards- compatibly) add `"rulesDirectory"` to `vrsource-tslint-rules`'s `tslint.json`, and then consumers can import the custom rules with `"extends"`. Note: doing an `"extends"` does bring in vrsource's configuration for all the built-in tslint rules, in addition to making the custom rules available. This normally shouldn't be a problem, as consumers can easily override the configuration they don't want (including by putting the `"vrsource-tslint-rules"` earlier in the `"extends"` array than a base `"tslint"` config they're using, like `"tslint:recommended"). However, where this is a problem, we can also make it possible to truly just import the custom rules. To do that, we can add an `index.js`/ `index.ts` file to the rules directory; as the TSLint docs point out (https://palantir.github.io/tslint/usage/configuration/), having an index file leads TSLint to use node's module resolution algorithm on the `"rulesDirectory"`. This means that consumers at any level can specify `"rulesDirectory": "vrsource-tslint-rules/rules"` and have it be resolved appropriately. This commit adds that index file too, and updates the readme accordingly.

Teamop · 2018-11-27T07:34:41Z

Hi @abierbaum , this PR deletes some rules like no-jasmine-focus, is it desirable? If it is, should be a breaking change from v5.8.2 to v5.8.3.

remyrylan · 2018-11-27T12:24:48Z

@Teamop I deleted only the generated files which no longer had any accompanying source file, so no-jasmine-focus was already not working.

Teamop · 2018-11-27T13:13:15Z

@jayrylan not really. Today after I updated from v5.8.2 to 5.8.3, then I got the error when linting:

Could not find implementations for the following rules specified in the configuration:
    no-jasmine-focus
Try upgrading TSLint and/or ensuring that you have all necessary custom rules installed.

If I downgraded to v5.8.2 again, it works

remyrylan · 2018-11-27T13:34:17Z

Ah, apologies about that @Teamop. It should be a breaking change then, you are correct. I'll defer to @abierbaum, since I'm not sure what the original reason was for deleting the source files of the rules (like I said, I only removed the generated files that had previously been compiled from source files).

Teamop · 2018-11-27T13:47:53Z

@jayrylan just as you said, I found the 7dec085 which deleted the source files for the no-jasmine-focus

abierbaum · 2018-11-27T18:10:07Z

Just pushed a new 6.0.0 release. I apologize for the oversight when 5.8.0 removed older rules.

I would welcome any PRs that add better build support to avoid this in the future.

remyrylan · 2018-11-27T18:16:38Z

@abierbaum I'm soon releasing a typescript compile tool aimed at libraries that wish to ship source files alongside generated code and would have caught this at build time. My company uses it in over 40 of our npm packages, so it will be supported for a long time. I'll make a PR here once I'm done with it.

…o `ban` The `no-jasmine-focus` rule has been removed from `vrsource-tslint-rules` [since version 5.8.0][1] (theoretically; practically it remained [until version 5.8.2][2]). This commit removes the non-existent rule (and the obsolete dependency) and uses tslint's `ban` rule instead (as recommended). [1]: vrsource/vrsource-tslint-rules@477f622#diff-04c6e90faac2675aa89e2176d2eec7d8R162 [2]: vrsource/vrsource-tslint-rules#13 (comment)

…o `ban` (angular#29926) The `no-jasmine-focus` rule has been removed from `vrsource-tslint-rules` [since version 5.8.0][1] (theoretically; practically it remained [until version 5.8.2][2]). This commit removes the non-existent rule (and the obsolete dependency) and uses tslint's `ban` rule instead (as recommended). [1]: vrsource/vrsource-tslint-rules@477f622#diff-04c6e90faac2675aa89e2176d2eec7d8R162 [2]: vrsource/vrsource-tslint-rules#13 (comment) PR Close angular#29926

Remy Rylan added 2 commits April 30, 2018 11:21

Cleanup, adhere to own ruleset and easier setup

44eb868

Make peerDependencies flexible.

b8d4db9

ethanresnick mentioned this pull request May 25, 2018

Add dummy index file; add rulesDirectory to tslint.json #14

Merged

abierbaum merged commit b8d4db9 into vrsource:master Nov 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup, adhere to own ruleset and easier setup #13

Cleanup, adhere to own ruleset and easier setup #13

remyrylan commented Apr 30, 2018 •

edited

Loading

Teamop commented Nov 27, 2018

remyrylan commented Nov 27, 2018

Teamop commented Nov 27, 2018 •

edited

Loading

remyrylan commented Nov 27, 2018

Teamop commented Nov 27, 2018

abierbaum commented Nov 27, 2018

remyrylan commented Nov 27, 2018

Cleanup, adhere to own ruleset and easier setup #13

Cleanup, adhere to own ruleset and easier setup #13

Conversation

remyrylan commented Apr 30, 2018 • edited Loading

Teamop commented Nov 27, 2018

remyrylan commented Nov 27, 2018

Teamop commented Nov 27, 2018 • edited Loading

remyrylan commented Nov 27, 2018

Teamop commented Nov 27, 2018

abierbaum commented Nov 27, 2018

remyrylan commented Nov 27, 2018

remyrylan commented Apr 30, 2018 •

edited

Loading

Teamop commented Nov 27, 2018 •

edited

Loading