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

refactor: simplify/improve effect types #913

Merged
merged 1 commit into from
May 3, 2022

Conversation

otonashixav
Copy link
Contributor

@otonashixav otonashixav commented Apr 1, 2022

  • Simplified createEffect etc. such that they no longer use rest params.
  • Added test cases for effect functions failing partial generic inference (not supported by typescript)
  • Removed extra params from effects' first overload

Edit: removed the changes not related to effect types, it would be better to clean them up all at once in another PR.

@otonashixav otonashixav force-pushed the refactor-effect-types branch 2 times, most recently from 58803c9 to 980d8a5 Compare April 1, 2022 20:48
@coveralls
Copy link

coveralls commented Apr 1, 2022

Pull Request Test Coverage Report for Build 2191771393

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.489%

Totals Coverage Status
Change from base Build 2171760479: 0.0%
Covered Lines: 1217
Relevant Lines: 1296

💛 - Coveralls

@otonashixav otonashixav force-pushed the refactor-effect-types branch 2 times, most recently from 4564834 to c9318cc Compare April 2, 2022 18:58
@otonashixav otonashixav marked this pull request as ready for review April 2, 2022 19:00
@otonashixav otonashixav marked this pull request as draft April 2, 2022 19:06
@otonashixav otonashixav force-pushed the refactor-effect-types branch 2 times, most recently from 8435b68 to 259cbef Compare April 2, 2022 19:28
@otonashixav otonashixav marked this pull request as ready for review April 2, 2022 19:30
@ryansolid ryansolid added the typescript relating to typescript or types label Apr 6, 2022
@otonashixav otonashixav marked this pull request as draft April 19, 2022 18:29
@otonashixav otonashixav force-pushed the refactor-effect-types branch 2 times, most recently from fe85bb8 to f091fbb Compare April 19, 2022 19:20
- Simplified `createEffect` etc. such that they no longer use rest args
- Added test cases for effect functions failing partial generic inference (not supported by typescript)
- Removed extra params from effects' first overload
@otonashixav
Copy link
Contributor Author

otonashixav commented Apr 19, 2022

@trusktr Just to let you know, I made another change to remove value and options from the first overloads of these functions, to bring them closer to how they were originally, and also make them consistent with the other instance of overloading, createSignal. This makes cases where undefined is explicitly passed use the second overload instead of the first. AFAICT this causes no issues, at least with the existing tests.

@otonashixav otonashixav marked this pull request as ready for review April 19, 2022 20:10
@ryansolid ryansolid changed the base branch from main to next May 3, 2022 03:04
@ryansolid ryansolid merged commit d62aa9d into solidjs:next May 3, 2022
@otonashixav otonashixav deleted the refactor-effect-types branch May 3, 2022 03:14
ryansolid added a commit that referenced this pull request May 12, 2022
* resource and reactive updates, fix #919

* stores: top level arrays, prevent obj overnotify, fixes #960 batching mutable

* fix #940 undefined ssr, fix #955 undefined className, fix #958 undefined spread

* changelog wip

* fix: fix the signal setter type when used with generics (#910)

This makes it easier to create generic wrappers around `createSignal` e.g.
```ts
function createGenericSignal<T>(): Signal<T | undefined> {
  const [generic, setGeneric] = createSignal<T>();
  const customSet: Setter<T | undefined> = (v?) => setGeneric(v);
  return [generic, (v?) => setGeneric(v)];
}

function createInitializedSignal<T>(init: T): Signal<T> {
  const [generic, setGeneric] = createSignal<T>(init);
  const customSet: Setter<T> = (v?) => setGeneric(v);
  return [generic, (v?) => setGeneric(v)];
}
```
where previously such the implementation would not be assignable to `Setter`, and would need to be cast.

Unexpectedly this also makes this more specific:
```ts
const [s, set] = createSignal<string>(); // <-- signal contains undefined
// before:
const v = set(() => "literal");
//    ^? - string
// after:
const v = set(() => "literal");
//    ^? - "literal"
```

* refactor: improve splitProps type for arbitrary rest args length (#930)

- Allows the return type of splitProps to be correct for 1 or more `...keys` passed
- Removes overloads for `splitProps` as they are no longer necessary
- Types each array passed as rest args as `readonly` as they do not need to be mutable

* v1.4.0-beta.0

* fix: spreading non-object props (#963)

- `createComponent` passes an empty object instead of non-object props which are spread
- `mergeProps` treats non-object sources as empty objects
- added tests

fixes #958

* Revised Component types (children, ref) (#877)

* Revised Component types (children, readonly, ref)

* Rename past `Component` type to `ComponentWithChildren`.
  Motivation: Most components don't actually support children.
  This causes a type error upon accidental passing of children /
  forces you to think about which components support children.
* Added second parameter to `ComponentWithChildren`
  to make it easier to give a custom type for `children`
  (but still defaults to useful `JSX.Element`).
* New `Component` type does not have automatic `children` property
  (like React's preferred `VoidFunctionalComponent`),
  offering another natural way to type `props.children`:
  `Component<{children: JSX.Element}>`.
* `props` argument in both `Component` and `ComponentWithChildren`
  automatically cast to `readonly` (shallow one level only),
  to avoid accidental assignment to `props.foo` (usually a getter)
  while still allowing passing mutables in props.
  Add `Props<T>` helper for this transformation.
* Add @lxsmnsyc's `Ref<T>` type so it's easy to type `props.ref`:
  `Component<{ref: Ref<Element>}>`.
  <#778 (comment)>
  Fixes #778.

* Fix test

* Remove Props helper and shallow Readonly for props

* VoidComponent, ParentComponent, FlowComponent

* Use Component<any> for generic component type

* Add default types, Context.Provider require children

Comments from Otonashi

* Default parameter for PropsWithChildren

* Restore missing <any>

* Docs typo fix

* Cleanup server code

* JSDoc improvements

* Improve FlowProps JSDoc

* More FlowComponent JSDoc improvements

Co-authored-by: Ryan Carniato <[email protected]>

* refactor: simplify effect types (#913)

- Simplified `createEffect` etc. such that they no longer use rest args
- Added test cases for effect functions failing partial generic inference (not supported by typescript)
- Removed extra params from effects' first overload

* optimize: use jest fake timer to test suspense (#964)

fix #761

* new store modifiers splice, modifyMutable, bug fixes

* v1.4.0-beta.1

* refactor: remove extra generic from modifyMutable (#965)

* fix splice length setting

* remove unnecessary splice

* feat: resource deferred streaming

* v1.4.0-beta.2

* support standard JSX transform

* fix #967 `onHydrated`

* v1.4.0-beta.3

* Different types for standard transform

* v1.4.0-beta.4

* fix solid hyperscript, startTransition cleanup

* fix build

* v1.4.0-beta.5

* fix synchronous resources

* more changelog updates

* fix batch consistency in stores

* v1.4.0-beta.6

* typescript: correct on helper types (#959)

* refactor: correct on helper types

- Previous input type now includes undefined, which is its initial value
- When defer is true, memos created with the on helper include undefined
- Fixed return type becoming unknown when passing a function with a third parameter (the previous type)
- Allow readonly arrays/tuples to be passed as deps
- Disallow empty arrays from being passed as deps
- Breaks backwards compatibility of the first generic; now it is the type of the inputs passed to the function instead of the type of the deps themselves:
```ts
// before
on<number>(() => 1, ...) // error
on<() => number>(() => 1, ...) // ok
on<[number]>([() => 1], ...) // error
on<[() => number]>([() => 1], ...) // ok

// now
on<number>(() => 1, ...) // ok
on<() => number>(() => 1, ...) // error
on<() => number>(() => () => 1, ...) // ok
on<[number]>([() => 1], ...) // ok
on<[() => number]>([() => 1], ...) // error
on<[() => number]>([() => () => 1], ...) // ok
```

* docs: update on helper jsdoc

* fix: missing readonly in on helper types' `AccessorTuple`

* refactor: allow any array in `AccessorTuple`

- So that passing arrays works
- Renamed to `AccessorArray`

Co-authored-by: Ryan Carniato <[email protected]>

Co-authored-by: Xavier Loh <[email protected]>
Co-authored-by: Erik Demaine <[email protected]>
Co-authored-by: WZT <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript relating to typescript or types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants