Convert sources reducer to ES modules #2736
Conversation
Without Flow complained about missing annotation. After setting it to `SourcesState` it complained about unrecognized methods, which I suspect come from Immutable and are not defined and `SourcesState`. Since this PR isn't about type-checking, I left it at `any`.
virzen
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2736 +/- ##
==========================================
- Coverage 60.66% 60.58% -0.08%
==========================================
Files 55 55
Lines 2148 2144 -4
Branches 434 434
==========================================
- Hits 1303 1299 -4
Misses 845 845
Continue to review full report at Codecov.
|
|
@virzen I haven't looked yet, but you might want to use the |
src/reducers/sources.js
Outdated
| @@ -44,7 +44,10 @@ const State = makeRecord( | |||
| }: SourcesState) | |||
| ); | |||
|
|
|||
| function update(state = State(), action: Action): Record<SourcesState> { | |||
| export function update( | |||
| state: any = State(), | |||
There was a problem hiding this comment.
Using state: Record<SourcesState> should work.
src/reducers/sources.js
Outdated
| @@ -44,7 +44,10 @@ const State = makeRecord( | |||
| }: SourcesState) | |||
| ); | |||
|
|
|||
| function update(state = State(), action: Action): Record<SourcesState> { | |||
| export function update( | |||
| state: any = State(), | |||
There was a problem hiding this comment.
I'm guessing that you could replace any with Record<SourcesState>. Can you try this?
There was a problem hiding this comment.
Done, indeed works! 😄
There was a problem hiding this comment.
oops looks like I was late 😅
There was a problem hiding this comment.
Hahaha, It's never too late :P
| const makeRecord = require("../utils/makeRecord"); | ||
| const { getPrettySourceURL } = require("../utils/source"); | ||
| const { prefs } = require("../utils/prefs"); | ||
| import * as I from "immutable"; |
There was a problem hiding this comment.
We can probably use import I from "immutable";
There was a problem hiding this comment.
This seems not to be the case:
src/reducers/sources.js:11
11: import I from "immutable";
^ Default import from `immutable`. This module has no default export.
There was a problem hiding this comment.
Oh. Thanks for correction
There was a problem hiding this comment.
Oh that's interesting. I wonder if this means we should only be importing the things we need from immutable in the future. Thanks for pointing this out and looking it up. 🥂
There was a problem hiding this comment.
Although there would be no significant gain in size (webpack 1.x bundles whole file anyway) I agree it's a good practice to do so. Also, it would be great for the eventual migration to webpack 2 (which features tree-shaking). 😄
|
🎉 No problem! |
* Convert reducers/sources to es modules * Convert tests for reducers/sources to es modules * Remove type for sources reducer's state * Set sources reducer's state argument's type to any Without Flow complained about missing annotation. After setting it to `SourcesState` it complained about unrecognized methods, which I suspect come from Immutable and are not defined and `SourcesState`. Since this PR isn't about type-checking, I left it at `any`. * Use Record notation for sources reducer's argument type
Associated Issue: #1884
Summary of Changes
anyto silence Flow errorNote about last point:
Without it Flow complained about missing annotation after addition of
exportstatement. After setting it toSourcesStateit complainedabout unrecognized methods, which I suspect come from Immutable.
Since this PR isn't about type-checking, I left it at
any, as suggestedin this comment.