Modularize #726

weswigham · 2015-10-12T11:21:46Z

Implements #465, #280.

This PR rearchitects tslint to be a series of modules rather than a bunch of merged namespaces meant to be concatenated together. What this has enabled is a handful of cool TS 1.6 things - for one, tslint now has typescript 1.6 compatible typings built alongside itself - meaning import * as Linter from "tslint"; (and import * as Lint from "tslint/lib/lint";) can now pull associated typings from the node package.

It does, however, retain the namespace structure tslint used before - it just creates it via modules now, rather than TS's namespace keyword. This means that, given the right entrypoint (namely tslint/lib/lint), the API surface should look identical to before this change.

This does contain a fairly large breaking change, however, but in doing so it fixes some odd things with the tslint build and makes it a bit less of a chore to integrate into your builds. For one, it no longer concatenates all of typescript into its built js, and for two, it no longer creates a global ts or Lint object (thereby mucking with people's build environments like so). Combined with the typings field support, this means external rules and formatters must replace their lines like this:

/// <reference path="../../node_modules/tslint/typings/typescriptServices.d.ts" />
/// <reference path="../../node_modules/tslint/lib/tslint.d.ts" />

with this:

import * as ts from "typescript";
import * as Lint from "tslint/lib/lint";

Additionally, this fixes #549, allows #532 to be done more easily.

adidahiya · 2015-10-12T15:31:33Z

awesome! we'll try to merge this this week and bump to 3.0 for the breaking change.

adidahiya · 2015-10-12T20:16:47Z

Gruntfile.js

            }
        }
    });

    // load NPM tasks
    grunt.loadNpmTasks("grunt-contrib-clean");
-    grunt.loadNpmTasks("grunt-contrib-concat");


let's remove grunt-contrib-concat from package.json as well

weswigham · 2015-10-13T08:18:34Z

@adidahiya Updated the PR.

jkillian · 2015-10-16T21:39:36Z

There's an issue with const enums and our next branch related to this as well. Currently, the next branch has a devDependency on `"typescript": "next", and with this modularization update, just a regular old dependency. But we have to be careful, if TSLint gets compiled with a TS nightly from Oct.15th and it gets installed on some later date, TSLint would then be using a newer version of TS to analyze files then it was compiled with, which could lead to enum value conflicts.

weswigham · 2015-10-16T21:47:42Z

Working on it.

jkillian · 2015-10-16T22:10:10Z

@weswigham Sweet!
@myitcv Release might be delayed a little, Github seems to be having issues for us today

adidahiya · 2015-10-27T14:53:50Z

released v3.0.0-dev.1

alexeagle · 2015-10-28T00:31:20Z

Trying this now, but something has changed with the way the top-level export works. Note the difference is the nesting under the 'default' property:

alexeagle-macbookpro2:use-tslint alexeagle$ node
> require('tslint');
{ default:
   { [Function: Linter]
     VERSION: '3.0.0-dev.1',
     findConfiguration: [Function: findConfiguration] } }
> alexeagle-macbookpro2:use-tslint alexeagle$ rm -rf node_modules/
alexeagle-macbookpro2:use-tslint alexeagle$ npm install tslint
tslint@2.5.1 node_modules/tslint
├── underscore.string@3.1.1
├── optimist@0.6.1 (wordwrap@0.0.3, minimist@0.0.10)
└── findup-sync@0.2.1 (glob@4.3.5)
alexeagle-macbookpro2:use-tslint alexeagle$ node
> require('tslint')
{ [Function: Linter]
  VERSION: '2.5.1',
  findConfiguration: [Function: findConfiguration] }

this causes gulp-tslint to fail:

[17:17:30] Starting 'lint'...

/Users/alexeagle/Projects/angular/node_modules/gulp-tslint/node_modules/rcloader/index.js:68
      cb(void 0, configFile);
      ^
TypeError: linter is not a function
    at /Users/alexeagle/Projects/angular/node_modules/gulp-tslint/index.js:94:22
    at respond (/Users/alexeagle/Projects/angular/node_modules/gulp-tslint/node_modules/rcloader/index.js:68:7)
    at respond (/Users/alexeagle/Projects/angular/node_modules/gulp-tslint/node_modules/rcloader/node_modules/rcfinder/index.js:140:7)
    at next (/Users/alexeagle/Projects/angular/node_modules/gulp-tslint/node_modules/rcloader/node_modules/rcfinder/index.js:164:16)
    at doNTCallback0 (node.js:407:9)
    at process._tickCallback (node.js:336:13)

jkillian · 2015-10-28T03:20:52Z

Thanks for the heads-up @alexeagle. The problem is that when you use the export default syntax in TS (as we do) it creates that default field (see the Default Export and Default Import section of this comment).

This is all fine and dandy when using TS ES6 imports, but it's a little messy when using CommonJS. Luckily there are ways to deal with this. I assume we'll do something like suggested in that previous link so CommonJS users don't have to change their code. What do you think @adidahiya?

alexeagle · 2015-10-28T03:55:44Z

Otherwise, this looks good to me. 👍
I have a PR out to add this to angular (didn't find any bugs by turning it on, sadly)
angular/angular#4970

Note you may want to document for the next release that custom rules need to use the same typescript version as tslint internally:
https://github.com/angular/angular/pull/4970/files#diff-dbb7291ecb65301ed3220de1c23eb9faR4
otherwise you get errors like

equireParameterTypeRule.ts(6,14): error TS2415: Class 'Rule' incorrectly extends base class 'AbstractRule'.
  Types of property 'apply' are incompatible.
    Type '(sourceFile: SourceFile) => RuleFailure[]' is not assignable to type '(sourceFile: SourceFile) => RuleFailure[]'.
      Types of parameters 'sourceFile' and 'sourceFile' are incompatible.
        Type 'ts.SourceFile' is not assignable to type 'ts.SourceFile'.
          Types of property 'statements' are incompatible.
            Type 'NodeArray<Statement>' is not assignable to type 'NodeArray<Statement>'.
              Types of property 'push' are incompatible.
                Type '(...items: Statement[]) => number' is not assignable to type '(...items: Statement[]) => number'.
                  Types of parameters 'items' and 'items' are incompatible.
                    Type 'ts.Statement' is not assignable to type 'ts.Statement'.
                      Types of property 'kind' are incompatible.
                        Type 'ts.SyntaxKind' is not assignable to type 'ts.SyntaxKind'.

jkillian · 2015-10-28T04:46:53Z

Yikes, that's a cryptic error. It's a little bit tricky now: when using the next branch of tslint, the version of TS used to compile tslint, provide the language services when tslint is running, and compile custom rules can all be different.

I think we ought to fix TS to a specific version for the next branch. So instead of the dependency being typescript: next, we ought to fix it to something like typescript: 1.8.0-dev.20151027 and then update that each week. This would make ensure that (TS version to compile tslint === TS version to run tslint).

That still leaves the issue of the version of TS used to compile custom rules. I think recommending that custom rules be compile with the same version of TS as tslint is using in the docs is a good starting point. If compiling the custom rules is integrated into your build, this is fine. It still makes it hard to use prebuilt-custom rules though.

We also have the issue of const enums that was mentioned earlier. @weswigham, is it correct that the TS .d.ts files will start having regular enums instead of const enums soon?

weswigham · 2015-10-28T04:52:03Z

Yes. We have an open PR with the change that'll likely be merged in soon.

On Tue, Oct 27, 2015, 9:46 PM Jason Killian notifications@github.com
wrote:

Yikes, that's a cryptic error. It's a little bit tricky now: when using
the next branch of tslint, the version of TS used to compile tslint,
provide the language services when tslint is running, and compile custom
rules can all be different.

I think we ought to fix TS to a specific version for the next branch. So
instead of the dependency being typescript: next, we ought to fix it to
something like typescript: 1.8.0-dev.20151027 and then update that each
week. This would make ensure that (TS version to compile tslint === TS
version to run tslint).

That still leaves the issue of the version of TS used to compile custom
rules. I think recommending that custom rules be compile with the same
version of TS as tslint is using in the docs is a good starting point. If
compiling the custom rules is integrated into your build, this is fine. It
still makes it hard to use prebuilt-custom rules though.

We also have the issue of const enums
microsoft/TypeScript#5297 that was mentioned
earlier. @weswigham https://github.com/weswigham, is it correct that
the TS .d.ts files will start having regular enums instead of const enums
soon?

—
Reply to this email directly or view it on GitHub
#726 (comment).

jkillian · 2015-10-28T05:12:15Z

(See microsoft/TypeScript#5422)

alexeagle · 2015-10-28T05:42:38Z

There is probably an issue where this discussion belongs.

Indeed this is tricky - we have similar problems with ts2dart but have the advantage that we control versions at tool-compile-time, user-compile-time, and runtime. Here's my somewhat-informed input:

If the const enum problem is resolved, then I won't have to care what TS you complied tslint with, I only need runtime compatibility with the JS files you distribute.
For type-checking in my compile step, ideally I think I should bring my own version of typescript, and it should be used for compiling my custom rules, along with other things I compile in my build toolchain. I just need to bring a version modern enough to understand the .d.ts files you distribute.
When tslint executes, I would also like it to use the TS language services at the version I bring. I need to use a modern enough version that the language services provide the APIs tslint calls.

myitcv · 2015-10-28T10:26:13Z

@alexeagle - thanks for adding comment about that version mismatch problem... just ran into exactly the same issue.

Just so that I'm clear, for people writing custom rules (incidentally we need a README update to reflect the merged #726), the advice is currently as follows:

import * as Lint from "tslint/lib/lint";
import * as ts from "tslint/node_modules/typescript";

export class Rule extends Lint.Rules.AbstractRule {
  // ...

Is that correct?

weswigham · 2015-10-28T20:02:00Z

import * as ts from "tslint/node_modules/typescript";

I'd avoid this. With npm3, there's a good chance that tslint's internal dependency gets flattened out to the top level.
I'd just use import * as ts from "typescript"; and specify a typescript version in your own package.json. tslint could even support more explicitly by requiring typescript as a peer dependency when used as a library. (Though, this would mean that tslint-cli and tslint would have different dependencies and would need to be split apart, as tslint-cli would need to require its own typescript compiler, and as of npm3, peer dependencies are not installed automatically.)

alexeagle · 2015-10-28T20:15:37Z

Too late, it's in angular master already :)!
angular/angular@c90e192

The problem is that 3.0.0-dev.1 is cut from the tslint@next branch so it
includes the TypeScript@next version. I need to depend on tslint@master so
the TS version will match the one in my node_modules.

On Wed, Oct 28, 2015 at 1:02 PM Wesley Wigham notifications@github.com
wrote:

import * as ts from "tslint/node_modules/typescript";

I'd avoid this. With npm3, there's a good chance that tslint's internal
dependency gets flattened out to the top level.
I'd just use import * as ts from "typescript"; and specify a typescript
version in your own package.json. tslint could even support more
explicitly by requiring typescript as a peer dependency when used as a
library. (Though, this would mean that tslint-cli and tslint would have
different dependencies and would need to be split apart, as tslint-cli
would need to require its own typescript compiler, and as of npm3, peer
dependencies are not installed automatically.)

—
Reply to this email directly or view it on GitHub
#726 (comment).

jkillian · 2015-10-28T22:41:39Z

I need to think about the best solution a little bit more - this discussion has been great so far, but I don't see a clear cut road forwards yet.

@weswigham Any recommended reading/documentation on previous/upcoming npm changes? I'm not too familiar with npm 3 vs. npm 2 and definitely not familiar with what the changes down the road will look like.

weswigham · 2015-10-28T22:58:05Z

@jkillian The npm roadmap, and the npm 3 changelog are great references.

For the record: npm 3+ is the latest stable version of npm - anyone doing an npm install npm gets it now. (Though node 4 shipped with npm 2.14.)

jkillian · 2015-10-29T15:30:18Z

With npm3, there's a good chance that tslint's internal dependency gets flattened out to the top level.

Thanks @weswigham, reading over the changelog and some related issues since npm v3.0.0 was helpful. I had initially thought you meant "with possible future changes to npm3" the dependency might get flattened, but in reality I see that npm3 can "choose" to install typescript as a sibling of tslint or it might install it as a child of tslint.

Having a non-deterministic location for typescript seems problematic; I like the idea of making it a peer dependency and thus always having it installed as a sibling. If we went down this road, I wonder what versioning requirement would be appropriate. Maybe >= to a specific version of TS?

Though, this would mean that tslint-cli and tslint would have different dependencies and would need to be split apart, as tslint-cli would need to require its own typescript compiler, and as of npm3, peer dependencies are not installed automatically.

Why would this be though? It seems to me they could stay as one package with a peer dependency on typescript. Yes, it would force people to install typescript besides tslint just in order to use the CLI, but this probably happens in almost all cases anyways.

jkillian · 2015-10-29T20:54:20Z

@alexeagle I'm going to change the behavior of the tslint library shortly, so you don't have to do the weird .default hack. See #760. This will make it into 3.0.0-dev.2 which will hopefully be released tomorrow

alexeagle · 2015-10-30T18:44:29Z

Any idea when a 3.0.0 will be cut from master (so it depends on TS 1.6.2?)

On Thu, Oct 29, 2015 at 1:54 PM Jason Killian notifications@github.com
wrote:

@alexeagle https://github.com/alexeagle I'm going to change the
behavior of the tslint library shortly, so you don't have to do the weird
.default hack. See #760 #760
This will make it into 3.0.0-dev.2 which will hopefully be released
tomorrow

—
Reply to this email directly or view it on GitHub
#726 (comment).

myitcv · 2015-11-02T22:20:14Z

I think we ought to fix TS to a specific version for the next branch. So instead of the dependency being typescript: next, we ought to fix it to something like typescript: 1.8.0-dev.20151027 and then update that each week.

👍

Just been bitten by this... right now I've had to switch our dependency to be on next... which is not great because it's a moving target - with multiple developers and a CI server, that's less than ideal.

myitcv · 2015-11-03T10:45:41Z

@jkillian looking forward to 3.0.0-dev.2 because as of TypeScript 1.8.0-dev.20151103 (i.e. today), tslint 3.0.0-dev.1 breaks. That's because of this change where the SyntaxKind enum is no longer defined as const.

The good news is that as of TypeScript 1.8.0-dev.20151029 (following the merge of microsoft/TypeScript#5422) rebuilding tslint 3.0.0-dev.1 just works, and the SyntaxKind etc enum values are not erased to integers.

jkillian · 2015-11-04T00:12:00Z

@myitcv Should be pretty soon: #768 :)
@alexeagle We want to make sure the recent changes are stable before 3.0.0 gets released. Release 3.0.0-dev.2 will fix the issue where you had to do require('tslint').default. If everything seems good with this dev release we'll release regular 3.0.0 next.

adidahiya · 2015-11-04T01:06:12Z

published 3.0.0-dev.2

myitcv · 2015-11-04T12:20:26Z

@adidahiya @jkillian - thanks very much, works like a charm.

adidahiya · 2015-12-01T03:57:29Z

FYI, the const enum problem is worse than I actually thought (but maybe y'all were aware). See #832.

Glad to see that microsoft/TypeScript#5422 got merged :)

weswigham added 9 commits October 2, 2015 06:08

Rearchitect lib, next up is tests

249c7d9

Merge branch 'master' into modularize

1a91560

almsot got builds/tests working

bcca064

fixed up all the paths, tests all pass

e8e3b3c

move ts to a real dependency

557c71b

Fix linter errors

9ef205f

remove built output

53c79e6

update gitignore

5230189

fix opts parsing

ae15fdc

adidahiya added the PR: waiting for reviewer label Oct 12, 2015

adidahiya reviewed Oct 12, 2015
View reviewed changes

adidahiya added PR: waiting for author and removed PR: waiting for reviewer labels Oct 12, 2015

myitcv mentioned this pull request Oct 13, 2015

tsfmt task #732

Closed

weswigham added 2 commits October 13, 2015 06:48

pr feedback

5299553

get the exports rejiggered, fix the lint

611a135

adidahiya added PR: waiting for reviewer and removed PR: waiting for author labels Oct 13, 2015

jkillian mentioned this pull request Oct 29, 2015

Rework Build System #761

Closed

jkillian mentioned this pull request Oct 30, 2015

Rework build system #762

Closed

jkillian mentioned this pull request Nov 11, 2015

Update npm package rules and .d.ts files #790

Closed

myitcv mentioned this pull request Nov 12, 2015

TypeScript version dependency for tslint@next #791

Closed

adidahiya mentioned this pull request Dec 1, 2015

Null pointer exception when using TypeScript 1.7.3 #832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize #726

Modularize #726

weswigham commented Oct 12, 2015

adidahiya commented Oct 12, 2015

adidahiya Oct 12, 2015

weswigham commented Oct 13, 2015

jkillian commented Oct 16, 2015

weswigham commented Oct 16, 2015

jkillian commented Oct 16, 2015

adidahiya commented Oct 27, 2015

alexeagle commented Oct 28, 2015

jkillian commented Oct 28, 2015

alexeagle commented Oct 28, 2015

jkillian commented Oct 28, 2015

weswigham commented Oct 28, 2015

jkillian commented Oct 28, 2015

alexeagle commented Oct 28, 2015

myitcv commented Oct 28, 2015

weswigham commented Oct 28, 2015

alexeagle commented Oct 28, 2015

jkillian commented Oct 28, 2015

weswigham commented Oct 28, 2015

jkillian commented Oct 29, 2015

jkillian commented Oct 29, 2015

alexeagle commented Oct 30, 2015

myitcv commented Nov 2, 2015

myitcv commented Nov 3, 2015

jkillian commented Nov 4, 2015

adidahiya commented Nov 4, 2015

myitcv commented Nov 4, 2015

adidahiya commented Dec 1, 2015

Modularize #726

Modularize #726

Conversation

weswigham commented Oct 12, 2015

adidahiya commented Oct 12, 2015

adidahiya Oct 12, 2015

Choose a reason for hiding this comment

weswigham commented Oct 13, 2015

jkillian commented Oct 16, 2015

weswigham commented Oct 16, 2015

jkillian commented Oct 16, 2015

adidahiya commented Oct 27, 2015

alexeagle commented Oct 28, 2015

jkillian commented Oct 28, 2015

alexeagle commented Oct 28, 2015

jkillian commented Oct 28, 2015

weswigham commented Oct 28, 2015

jkillian commented Oct 28, 2015

alexeagle commented Oct 28, 2015

myitcv commented Oct 28, 2015

weswigham commented Oct 28, 2015

alexeagle commented Oct 28, 2015

jkillian commented Oct 28, 2015

weswigham commented Oct 28, 2015

jkillian commented Oct 29, 2015

jkillian commented Oct 29, 2015

alexeagle commented Oct 30, 2015

myitcv commented Nov 2, 2015

myitcv commented Nov 3, 2015

jkillian commented Nov 4, 2015

adidahiya commented Nov 4, 2015

myitcv commented Nov 4, 2015

adidahiya commented Dec 1, 2015