Skip to content

Conversation

@sgrishchenko
Copy link

@sgrishchenko sgrishchenko commented Jan 19, 2019

Feature scope:

  • dependencies from Expose selector dependencies for testing purposes #251 added in typings
  • typescript and typings-tester versions updated (and tests fixed after that)
  • improved type checking ("strict": true in tsconfig.json)
  • added code generator for boilerplate code of createSelector overloads
  • added formatting of generated typings with prettier (see compile:typescript in package.json)
  • added type checking of generated typings with tsc --noEmit
  • removed homogeneous selectors (heterogeneous selectors completely cover all cases)
  • CI workflow changed (now types are being tested after compilation)

Minor changes:

  • dropped CI build for Node v0.12, v4 and v5 (for code generation used ts-simple-ast which depends on fs-extra and fs-extra used many es6 features)
  • added CI build for Node v8, v9 and v10

@codecov
Copy link

codecov bot commented Jan 19, 2019

Codecov Report

Merging #393 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #393   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          15     15           
=====================================
  Hits           15     15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c93252d...547e266. Read the comment docs.

@simperreault
Copy link

simperreault commented May 30, 2019

Any idea when this pull request is expected to be merged ? I'm really looking forward using the typing in my typescript unit tests.

In the meantime (if it can help others) I added this to my code so that I do not get typings error:

function hasDependenciesProperty<S, R, C>(selector: OutputSelector<S, R, C>): selector is OutputSelector<S, R, C> & { dependencies: Function[] } {
    return selector.hasOwnProperty('dependencies');
}

export function getDependencies<S, R, C>(selector: OutputSelector<S, R, C>): Function[] {
    if (hasDependenciesProperty(selector)) {
        return selector.dependencies;
    }

    return [];
}

@anilanar
Copy link

👎 Those types obviously won't work. Parametric selectors are never uniform.

@sgrishchenko
Copy link
Author

sgrishchenko commented Dec 14, 2019

@anilanar what did you mean? Unirorm it is homogenios or geterogenios? Why will not this typings working? There is special typing test, that checks typings. If you have broken code, can you send code snippet to check it?

@anilanar
Copy link

anilanar commented Dec 14, 2019

Sorry I was hasty when I read the code, those types are pretty good.

One problem for us though is that having a third parameter in a child selector still produces a selector whose third arg is any; which is not acceptable for us. So we have a fork that doesn't allow more than 2 parameters.

@sgrishchenko
Copy link
Author

Are you saying about this ...args: any[] in parametric selector?

@anilanar
Copy link

Yes, exactly.

@sgrishchenko
Copy link
Author

Ou, I can not say that it is good solution for this library maintainers, but I think it is not linked with main goal off this PR. I just wanted to add dependecies in typings but when I tried to do this I was surprised how difficult to do this. So, I tried to simplify typings maintaining. But almost an year has passed and there's no activities from maintainers. They could say that they don't like solution with typings generation, I can prepare PR with hargcoded dependencies in typings. But if we are speaking about any parameters, I left them as is, because removing of them is breaking changes. But i didn't want to make breking changes, i wanted to make some small improvement. If you want to suggest removing of this any parameters, you can make separete PR with realisation of this suggestion. But thanks for activity in this PR, may be it will attract attention of maintainers.

@anilanar
Copy link

@sgrishchenko If you publish a fork, I'd gladly use your typings.

@markerikson markerikson added TypeScript duplicate This issue or pull request already exists labels Oct 17, 2021
@markerikson
Copy link
Contributor

We've been able to update the TS infrastructure and migrate to TS in #486 and #511. Thanks anyway!

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

Labels

duplicate This issue or pull request already exists TypeScript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants