Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Convert remaining of reducers to ES modules #2739

Merged

Conversation

virzen
Copy link
Contributor

@virzen virzen commented Apr 25, 2017

Associated Issue: #1884

Summary of Changes

Converted following reducers to ES modules:

  • async requests,
  • breakpoints,
  • coverage,
  • index,
  • pause,
  • ui.

All reducers has been updated to export update reducer by default. This allowed to somewhat simplify main reducer (reducers/index).

Also, type definitions for update functions' state argument have been added.

Another small change is addition of yarn flow command to the yarn test-all one. Lack of the former in the latter proved to be somewhat inconvenient when I wanted to manually check if everything is fine - the shortest way was yarn test-all && yarn flow. I completely agree this change is unrelated to the topic of this PR and won't mind to revert it if it's a problem.

@virzen virzen changed the title Convert the rest of reducers to es modules Convert remaining of reducers to ES modules Apr 25, 2017
@@ -28,7 +28,7 @@
"lint-fix": "yarn lint-js -- --fix",
"test": "jest",
"test-coverage": "yarn test -- --coverage",
"test-all": "yarn test; yarn lint",
"test-all": "yarn test; yarn lint; yarn flow",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see note at the bottom of PR description addressing this.

@codecov
Copy link

codecov bot commented Apr 25, 2017

Codecov Report

Merging #2739 into master will decrease coverage by 0.48%.
The diff coverage is 57.89%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2739      +/-   ##
=========================================
- Coverage   60.58%   60.1%   -0.49%     
=========================================
  Files          55      54       -1     
  Lines        2144    2118      -26     
  Branches      434     434              
=========================================
- Hits         1299    1273      -26     
  Misses        845     845
Impacted Files Coverage Δ
src/reducers/async-requests.js 91.66% <ø> (-0.65%) ⬇️
src/reducers/expressions.js 97.91% <ø> (ø) ⬆️
src/reducers/sources.js 78.22% <ø> (ø) ⬆️
src/utils/test-head.js 60% <ø> (-1.91%) ⬇️
src/reducers/event-listeners.js 22.22% <100%> (ø) ⬆️
src/reducers/pause.js 14.08% <28.57%> (-4.59%) ⬇️
src/reducers/coverage.js 46.15% <60%> (-10.1%) ⬇️
src/reducers/breakpoints.js 78.35% <66.66%> (-1.27%) ⬇️
src/reducers/ui.js 83.72% <88.88%> (-0.73%) ⬇️

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 0eaef81...95a3864. Read the comment docs.

@jasonLaster
Copy link
Contributor

re test-all. Agreed! We've wanted to do this.

Copy link
Contributor

@jasonLaster jasonLaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

return `${location.sourceId}:${location.line}`;
}

function allBreakpointsDisabled(state) {
return state.breakpoints.every(x => x.disabled);
}

function update(state = State(), action: Action) {
function update(state: Record<BreakpointsState> = State(), action: Action) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe export default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered about this and in the end I followed the pattern that seems to be present in majority of the components - component is defined in one place, but exported by default at the end of the file. Do you think it would be better to place export immediately before the reducer?

coverage: coverage.update
pause,
ui,
coverage
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@jasonLaster
Copy link
Contributor

stop by our slack room :)

@jasonLaster
Copy link
Contributor

We'd love to switch to webpack 2, but got stuck last time. We should have the branch lying around though

@jasonLaster
Copy link
Contributor

Don't worry about coverage. That always happens with import. I don't know why. I'll drop the limits if I need to

@virzen
Copy link
Contributor Author

virzen commented Apr 25, 2017

Re slack room: definitely!

Re webpack: I might take a look at this in some not-so-distant future then, but I can't promise anything. 😄

Re coverage: Great! 👌 (Of course it's great that I don't need to worry, not that it happens 😄 )

@jasonLaster jasonLaster merged commit d5285e0 into firefox-devtools:master Apr 25, 2017
@jasonLaster
Copy link
Contributor

Merging as this is too good to pass up.
We can always tweak the exports and I don't want any conflicts to emerge.

DanUgelow pushed a commit to DanUgelow/debugger.html that referenced this pull request May 4, 2017
* Convert async-request reducer to ES modules

* Convert breakpoints reducer to es modules

* Convert coverage reducer to es modules

* Convert pause reducer to es modules

* Convert main reducer to es modules

* Convert ui reducer to es modules

* Export update function of reducers by default

* Set proper types for reducers' state argument

* Add flow type-checking to `test-all` command
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants