-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
@@ -72,7 +72,7 @@ withFooAndBarProps = withFooAndBar.Props; | |||
declare const C2: React.ComponentType<typeof withFooAndBar.Props>; | |||
const WithFooAndBar = withFooAndBar(C2); | |||
|
|||
// $ExpectError Property 'token' is missing in type '{}'. | |||
// $ExpectError Property 'token' is missing in type '{}' but required in type '{ token: string; }'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes should fix the CI but for some reason, the old version of the file seems to still run..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I'm not sure if we want to maintain this type checks
in the future, do they really bring value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes should fix the CI but for some reason, the old version of the file seems to still run..
meaning it doesn't fail on your machine but it fails on the CI without these changes? Looks like a different version of TS or related libraries issue?
also I'm not sure if we want to maintain this type checks in the future, do they really bring value?
I would say yes until the d.ts
are written manually - too easy to break them otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meaning it doesn't fail on your machine but it fails on the CI without these changes? Looks like a different version of TS or related libraries issue?
yesterday the CI was failing with comments related to the "pre-changes" code, today everything works fine 👍
src/queries.js
Outdated
@@ -3,7 +3,7 @@ import debug from 'debug'; | |||
import shallowEqual from 'buildo-state/lib/shallowEqual'; // TODO(split) | |||
import pick from 'lodash/pick'; | |||
import _displayName from './displayName'; | |||
import 'rxjs/add/operator/debounceTime'; | |||
import { debounceTime } from 'rxjs/operators'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can read about rxjs
changes here: https://www.academind.com/learn/javascript/rxjs-6-what-changed/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will probably require to have rxjs upgraded in avenger
too. Otherwise we'll end up having two different versions of the lib (and it will probably break)
Maybe we can postpone this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works with "avenger": "^4.0.3"
but I agree that I don't like having two different versions of rxjs
.. maybe the solution could be to drop rxjs
in react-avenger
(it is only used here I think...)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM except for a couple inline comments
yarn.lock
Outdated
@@ -0,0 +1,4910 @@ | |||
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is a library repo, yarn.lock
shouldn't be versioned
package.json
Outdated
@@ -56,9 +52,10 @@ | |||
"react": ">=0.14.9 <15 || >=15.3.x" | |||
}, | |||
"dependencies": { | |||
"@devexperts/remote-data-ts": "^0.4.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(still) unrelated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ops, this shouldn't be here :)
@@ -3,3 +3,4 @@ npm-debug.log | |||
lib | |||
.eslintcache | |||
package-lock.json | |||
yarn.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this line (not sure but it seems legit if we don't want to version the file)
Closes #73
Closes #74
Test Plan
tests performed