Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scripts to generate DT compatible definition/test files #191

Open
3 of 5 tasks
ikatyang opened this issue Aug 15, 2017 · 12 comments
Open
3 of 5 tasks

Add scripts to generate DT compatible definition/test files #191

ikatyang opened this issue Aug 15, 2017 · 12 comments
Assignees

Comments

@ikatyang
Copy link
Member

ikatyang commented Aug 15, 2017

WIP at add-scripts-to-generate-dt-compatible branch.

Progress:

  • generated structure should look something like:
    /
      index.d.ts
      src/
        *.d.ts
      ramda-tests.ts (remove failing tests for dts-lint if needed, (optional) combine all unit tests into one file)
      tslint.json (extends: dtslint/dt.json)
      tsconfig.json (files: index.d.ts, ramda-tests.ts, src/*.d.ts)
    
  • add a file for listing maintainers information (to generate DT's header, copied from DT first).
  • run prettier for generated files first to prevent linting error from dts-lint
  • pass the dtslint test ($ExpectType/$ExpectError)
  • test tests from DT before making a PR to DT, make sure no regression from DT. (should be another issue)
  • [FUTURE] maybe write some scripts to auto make PR to DT?!
@ikatyang ikatyang self-assigned this Aug 15, 2017
@KiaraGrouwstra
Copy link
Member

See DefinitelyTyped/DefinitelyTyped#15912, at the time I made the needed adjustments but got stuck on failing tests.

@ikatyang
Copy link
Member Author

I haven't tried the $ExpectError from dtslint, if it can handle failing tests properly then it'd be easy to transform, otherwise I'll remove those failing tests while transforming, thanks for the information. 👍

@ikatyang
Copy link
Member Author

ikatyang commented Aug 17, 2017

The $ExpectError/Type behaves weird in some cases (travis log).

  • $ExpectError does not suppress any error in the generated test cases, but it did suppress errors in my local test repo, how could it happen?!

  • $ExpectType works well in most cases

    • but it threw in this case
      // $ExpectType boolean
      R.or(false, true); //=> true
      // $ExpectType 0 | never[]
      R.or(0, []); //=> []
      // $ExpectType number | never[]
      R.or(0)([]); //=> []
      // $ExpectType "" | null
      R.or(null, ""); //=> ''
      with outputs
      ERROR: 1979:5   expect  Expected type to be:
          0 | never[]
      got:
          or_11<0, never[]>
      ERROR: 1981:5   expect  Expected type to be:
          number | never[]
      got:
          or_11<number, never[]>
      ERROR: 1983:5   expect  Expected type to be:
          "" | null
      got:
          or_11<null, "">
      
    • and it did not throw errors with the same kind of test for R.and
      • test cases
        // $ExpectType boolean
        R.and(false, true); //=> false
        // $ExpectType 0 | never[]
        R.and(0, []); //=> 0
        // $ExpectType number | never[]
        R.and(0)([]); //=> 0
        // $ExpectType "" | null
        R.and(null, ""); //=> null
      • R.or and R.and returns the same type (T | U), how weird...

Still finding what the cause is 😭

@KiaraGrouwstra
Copy link
Member

That's odd, yeah. Then again, I guess the number of cases where we'd really wanna verify errors is kinda limited, right?
Does it also throw the or_11 thing on the distributions without placeholder?

@ikatyang
Copy link
Member Author

There's only 7 failing tests, but I'd like to keep them alive unless there's no solution.

And I tried dist-simple with the same test cases, got the same result.


Interestingly, if I cut them down:

import * as R from "ramda";

// tslint:disable comment-format

// @dts-jest:group and
(() => {
    // $ExpectType boolean
    R.and(false, true); //=> false
    // $ExpectType 0 | never[]
    R.and(0, []); //=> 0
    // $ExpectType number | never[]
    R.and(0)([]); //=> 0
    // $ExpectType "" | null
    R.and(null, ""); //=> null
})();

// @dts-jest:group or
(() => {
    // $ExpectType boolean
    R.or(false, true); //=> true
    // $ExpectType 0 | never[]
    R.or(0, []); //=> []
    // $ExpectType number | never[]
    R.or(0)([]); //=> []
    // $ExpectType "" | null
    R.or(null, ""); //=> ''
})();

The output will become:

ERROR: 10:5  expect  Expected type to be:
  0 | never[]
got:
  and_11<0, never[]>
ERROR: 12:5  expect  Expected type to be:
  number | never[]
got:
  and_11<number, never[]>
ERROR: 14:5  expect  Expected type to be:
  "" | null
got:
  and_11<null, "">
ERROR: 22:5  expect  Expected type to be:
  0 | never[]
got:
  and_11<0, never[]>
ERROR: 24:5  expect  Expected type to be:
  number | never[]
got:
  and_11<number, never[]>
ERROR: 26:5  expect  Expected type to be:
  "" | null
got:
  and_11<null, "">

If I place R.or before R.and, then the output will become errors for R.or.

It seems order matters...

@KiaraGrouwstra
Copy link
Member

well, if commenting the error tests would help get us a DT PR in, then whatever I guess.
That order thing is weird, yeah. I knew that could happen with interface overloads, but not with named types...
I guess the other mystery is why the union types won't unwrap though. I wonder if an alternative could be to have the all-1 types could be in-lined so it'd just directly do this:

type and_10<T> = {
    <U>(b: U): T | U;
};

@ikatyang
Copy link
Member Author

I found the reason why union types didn't unwrapped, they did not pass the enclosingDeclaration parameter to typeToString (dtslint vs dts-jest), I'd like to open a PR there but I think they won't accept, haha. It seems I should inline those all-1 types in my dts-element-fp, though it might increase the size of generated files.

@KiaraGrouwstra
Copy link
Member

Why not? Try opening an issue with them explaining the problem to get their take on why they passed undefined at least, hopefully for them passing it wouldn't break anything. :)

@ikatyang
Copy link
Member Author

Opened issue microsoft/dtslint#57, hope they would accept the changes.

@treybrisbane
Copy link

Is this still happening?

Is there still uncertainty on how to proceed here, or is the path clear and it's just a matter of actually doing it?

@ikatyang
Copy link
Member Author

The infrastructure in this repo is somehow hard to understand, even I as the author sometimes forgot why I wrote some code in that way, I think it's time to rewrite those scripts and also simplify the codegen using some conditional types so that it'd be easier for people to contribute. This PR will be done after it.

@treybrisbane
Copy link

Fair enough. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants