refactor(compile): Activate typescript strict features

[Sinewyk]

Just a cleanup in preparation of trying to fix https://github.com/staltz/xstream/issues/262

Actual line that we need to make compile after: https://github.com/Sinewyk/xstream/blob/2299e8f3441ed3b0723e830cfd2000b684858faa/tests/operator/flatten.ts#L338-L341

[staltz]

Thanks @Sinewyk, this looks like a carefully written PR. I have two comments, I'll add them inline.

Also does @jvanbruegge have comments?

[Sinewyk]

One thing I think for sure is extraneous in this PR, is all the /// <reference types="node" /> comment I deleted. Correct me if I'm wrong, but AFAIK they are old, for when typescript did not know how to find them by itself, so they aren't necessary anymore.

But I did not need to delete them to make everything compile.

[Sinewyk]

I guess I'll just squash the latest commit then, even though it's green on my machine.

[webmaster128]

Great stuff! This should also close https://github.com/staltz/xstream/pull/233, right?

[Sinewyk]

Oh, and I could improve the typing of flatten, by reading #233 I saw flatten<R, S extends Stream<R>>(this: Stream<S>): Stream<R>, that was something I was looking for but did not know how to express at the time.

PS: also closes #231 ?

[Sinewyk]

Playing around with https://github.com/Sinewyk/xstream/commit/ef27baccac41bc84bd1763badccd9a200d83436b (this branch + another commit, didn't want to pollute this PR with some experiments) to "improve" the flatten operator, around the idea:

flatten<R, S extends Stream<R>>

It works except the tests/operator/flatten.ts, I have tons of

The 'this' context of type 'Stream<Stream<number>>'
is not assignable to method's 'this' of type 'Stream<Stream<{}>>

so I can't quite get the type to be inferred by typescript correctly.

Only the third line ends up "good"

(method) Stream<Stream<number>>.flatten<number, Stream<number>>(this: Stream<Stream<number>>): Stream<number>

The second line (the most important because of cyclejs HTTP.select().flatten()) is

(method) Stream<MemoryStream<number>>.flatten<{}, Stream<{}>>(this: Stream<Stream<{}>>): Stream<{}>

we can make it work by supplying type parameters, but I'm not satisfied (and it would be a breaking change ?), any ideas ?

[staltz]

@Sinewyk It's a good idea to keep PRs focused on solving one thing at a time, e.g. I'm happy to review other PRs if you have other fixes here and there. For this one, it's important to do only what the commit message is saying and nothing else: 'activate typescript strict features'. So, for instance, activate it in tsconfig.json, and do the necessary changes in the source file. The tests files should not change, unless they simply do not pass, which means that there is a breaking change. Ideally, any change to tests is either: an addition (new tests), a code style refactor, or a breaking change. And for each of those three cases, the commit message should clearly indicate what is going on. Note also that we use the commit message to automatically detect what the next version should be, major or minor.

I think this PR would probably benefit from having a tsconfig.json separately for the tests, and the necessary changes to the npm script so that it uses the new tsconfig.json. By using a strict tsconfig for the implementation, and a loose tsconfig for tests, we can verify that xstream TypeScript is meant not only for users that setup strict features, but also for people who use the loose mode in TypeScript. JavaScript use is the loosest of all, but we don't need to test that in compile-time, obviously, just runtime.

[jvanbruegge]

@staltz strict mode is a subset of loose mode. When it compiles in strict modr, it will also compile in loose mode

[staltz]

@jvanbruegge Yes I understand that for the implementation, but strict mode was not being applied to tests before (which should reflect not the implementation, but the expected and supported use cases), and it should stay like that. This is not just about correctness, it's also about what the commit message communicates.

[jvanbruegge]

But isnt that the goal of this PR? Making HTTP.select().flatten() work with xstream, aka usage, not implementation?

[staltz]

Yes, it can be one of the goals, as long as it's done explicitly. For instance, if the goal is to support that API with xstream, then we should add a new test that reflects that use case, instead of changing all of the tests in a way that implicitly or not clearly indicates support for this specific use case.

[Sinewyk]

Do we need to have two tsconfig.json then ? One for the source code in strict, one for the tests in loose ?

[staltz]

@Sinewyk, if possible, yes

[Sinewyk]

I'm not happy with this, I don't know why yet.

Random: I think xstream should be pulled into the cyclejs monorepo, it was created for it, it should live with it.

[mightyiam]

I have used xstream without Cycle.js before.

[staltz]

I'm not happy with this, I don't know why yet.

@Sinewyk I'm ready to merge this, are you okay with merging? :)

[Sinewyk]

Sure, go for it. Here's to hoping I didn't mess something up :beers:

random: I use the refined github extension (firefox at least) that move some things around, so it's a lot more difficult (impossible?) to close issues/PR by mistake

[staltz]

Alright, thanks a lot for the PR and for the patience! Well done :)

random: I use the refined github extension (firefox at least) that move some things around, so it's a lot more difficult (impossible?) to close issues/PR by mistake

Just in case you're wondering, I did that to manually trigger a Travis build.