Apparently, some TS changes make some code irrelevant?

[StoneCypher]

In #5, @Krumpet said

That sounds great. Do note that along with the reformatting, some segments are no longer relevant as they were related to a TS issue that was open at the time and has since been resolved - I haven't tested but the code could be made simpler.

I would like to learn more

[StoneCypher]

Also mentioned this in #1

[Krumpet]

In my original blog post, this:

But we get this error: Type 'T' cannot be used to index type 'typeMap'. In the case where T is not a constructor type, the compiler still doesn't narrow T down to keyof typeMap, and so tells us that we cannot safely use T as an index of typeMap. We will see this problem again later, it's an open issue that I feel is worth mentioning. I'll expand on it in an addendum.

and the entire addendum are both about an issue with generic type parameters extending union types not being narrowed. So if you had x: X|Y you could do if (isX(x)) { // x is X here } else { // x is Y here }, BUT if you had <T extends X | Y> and x: T then the type of x wouldn't be narrowed. Since this issue has since been marked as resolved maybe the code can be cleaned up and still work.

[Krumpet]

Specifically I believe this code:

    const localPrimitiveOrConstructor: PrimitiveOrConstructor = className;
    if (typeof localPrimitiveOrConstructor === 'string') {
    return typeof o === localPrimitiveOrConstructor;

can be simplified to just use className directly without assigning to a local variable.

[StoneCypher]

I'm happy to fix it, or you could if you want your name more on the codebase

I will assume that what you choose here will be your general preference

[Krumpet]

Hi, I cloned the repo and tried making the changes I talked about but it looks like the bug still exists in this instance. I'll try commenting in my original SO question and on the "closed" issue.

I did make some changes in the file and the tests are still passing so I'll create a PR.

I did run into a problem running all of the test script since it uses ls and I'm on a windows machine at home.

[Krumpet]

here is my question on the closed issue

[StoneCypher]

I did run into a problem running all of the test script since it uses ls and I'm on a windows machine at home.

This is surprising. Where does it do so? I will try to be more portable.

I'm also on a Windows machine, but I use git bash, so this hasn't impacted me

Your branch builds on Windows in CI

[Krumpet]

Just for reference (probably belongs in a different place, here is my original SO question that lead to this whole thing.

[Krumpet]

I did run into a problem running all of the test script since it uses ls and I'm on a windows machine at home.

This is surprising. Where does it do so? I will try to be more portable.

I'm also on a Windows machine, but I use git bash, so this hasn't impacted me

Your branch builds on Windows in CI

'ls' is not recognized as an internal or external command,
operable program or batch file.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] test: `rimraf -f coverage && jest --color --coverage --verbose && ls -la coverage && ls -la coverage/spec && echo jest -c jest-stoch.config.js --color --verbose`npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\ran.lottem\AppData\Roaming\npm-cache\_logs\2021-07-26T19_51_58_219Z-debug.log

I ran npm run test

[StoneCypher]

oh for

those are debugging stubs from when i was struggling to sort out a timing problem in CI

fixing 😅 sorry

[StoneCypher]

fixed in 0.14.1

0ed4232

[StoneCypher]

I believe @Krumpet fixed the base issue in d35d6d2

If you could confirm and close please?

[Krumpet]

The changes ended up just being some type aliases, I did not make the changes I thought I could do because it looks like the issue with "type variable extending union type is not narrowed" still persists. I believe there is a way to rewrite to simplify the code, I just haven't figured it out yet.

[Krumpet]

I think I got it!

The answer here lies in making the implementation simpler, while delegating the 'type-correctness' to overloaded function declarations, so everything is inferred correctly while the implementation stays simple. Hopefully I can get a PR online tonight ( = in a few hours).

function typeGuard<T extends Constructor>(o: unknown, className: T): o is ConstructedType<T>;
function typeGuard<T extends keyof typeMap>(o: unknown, className: T): o is typeMap[T];


// finally, guard ALL the types!
function typeGuard(o: unknown, className: PrimitiveOrConstructor) {

  if (isPrimitiveName(className)) {
    return typeof o === className;
  }

  return o instanceof className;
}


function isPrimitiveName(className: PrimitiveOrConstructor): className is keyof typeMap {
    return (typeof className === 'string')
}

[StoneCypher]

Merjt.

Following f09e77a, does this now feel open or closed to you?

[Krumpet]

Closed, yeah.

…

On Wed, Jul 28, 2021, 22:38 John Haugeland ***@***.***> wrote: Merjt. Following f09e77a <f09e77a>, does this now feel open or closed to you? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEUXSXNDX2WLCPPMMONGYALT2BMDNANCNFSM5A62C63Q> .

[StoneCypher]

Fixed in f09e77a