Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Resolve localTs path #229

Merged
merged 1 commit into from
May 16, 2019
Merged

Resolve localTs path #229

merged 1 commit into from
May 16, 2019

Conversation

cartant
Copy link
Contributor

@cartant cartant commented Apr 19, 2019

This PR calls resolve on the argument supplied to the localTs command-line option - so that a path relative to the current directory can be passed.

ATM, it's necessary to pass an absolute path, as the dtslint implementation passes the supplied path to require in files within two different directories - so it's not possible to pass a relative path that works with both locations.

The need to pass an absolute path necessitated some shenanigans in RxJS, where we use dtslint to test type declarations:

The command must be passed an absolute path to the directory that contains typescript.js as said path is passed to require in two files within the dtslint implementation that reside in different directories - so each has a different relative path the the locally-installed TypeScript. To account for this, dtslint is now invoked via spec-dtslint/script.js.

This would be considerably simpler if a relative path could be passed.

@sandersn
Copy link
Member

Why does RxJS use localTs? The intent of dtslint is normally to test all versions of Typescript to provide evidence that users of all those versions won't be broken.

We added localTs to make it possible for us to test Typescript on Definitely Typed, which is backwards from the normal case.

@cartant
Copy link
Contributor Author

cartant commented May 14, 2019

dtslint is not being used to lint an RxJS .d.ts file. Well, not a manually authored one. It's being used to perform inference expectations - using the $ExpectType rule.

dtslint is used to run a suite of tests that assert that the types are inferred correctly and that the overload signatures are working as they should.

A large portion of the RxJS API involves the use of many, many overload signatures. And in the past seemingly small. inconsequential changes have turned out to be breaking. Or - worse - have seen types inferred as any.

Perhaps it would have been better to simply build tooling around the $ExpectType rule itself, but at the time having dtslint perform the tests with a range of TS versions seemed worthwhile. However, more recent - next - versions of TS have increasingly seen the inference behaviour change, effecting failures for the $ExpectType expectations.

ATM, the simplest change - for us - would be to resolve the local path and leave the tooling as is.

@cartant
Copy link
Contributor Author

cartant commented May 15, 2019

Specifically, the problem was that the expect rule saw unknown inferred in next and {} inferred for earlier versions and that caused problems with these tests.

@sandersn
Copy link
Member

@weswigham, opinions on this? I think you added this feature.

@sandersn
Copy link
Member

I'll go ahead and merge since it seems like the right thing. I'll publish a new version and test the run dt behaviour on some current PR.

@sandersn sandersn merged commit 1daede3 into microsoft:master May 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants