-
Notifications
You must be signed in to change notification settings - Fork 4
Create slices #21
base: master
Are you sure you want to change the base?
Create slices #21
Conversation
Fix processing of tsx files by SWC
Conform tsconfig.json to formatting rules Update config Allow for subfolder typescript linting Conform to eslint rules Remove redundant fallthrough rule Include tsRules for tsx files Fix react in jsx scope linter warnings Remove unecessary propTypes check due to use of TS
Ensure import order, and fix import filetype Conform to eslint rules Fix import order Conform to eslint rules Fix indentation Fix formatting
Create Model Slice Use EnhancedStore Use root level imports, not pwd paths. Clarify naming overlap between store definition and params Add types for modelSlice reducers Export modelSlice actions Export accurate RootState and AppDispatch types Unify model reducer action types Create windSlice.ts Create skewtSlice Rename skewt pMin action type Create SkewtState Create metricSlice.ts Consistently type metricSlice initialState Update metric types Specify modelSlice types Allow for list of params, convert to payload object Move reducers to slices Create pluginSlice Update setModelName import path for new slice structure Move setModelName to modelSlice Create cancelSubscription thunk Update updateMetrics import path Move updateMetrics from store.ts to metricSlice Revert modelName filtering correctly Remove outdated redux actions and reducers Convert LoadingIndicator to functional component Import actions from new pluginSlice Fix store import to include typing Include pressure in MetricState Import actions from pluginSlice, not old actions file Access store by import rather than getter function Explicitly type debugClouds to avoid poor inference modify state selectors to match slice state naming Reconfigure store definition, for cleaner importing into components Rename import locations using root relative paths Futureproof flexibility of typing receiveParams reducer Prefer parameters object over args Add typing to selectors to prevent malformed accessing of state Match updateMetrics to thunk pattern Remove unnecessary anonymous functions Dispatch model name updates Fix improper typing of modelName due to 'as const' on model prefixes rename getStore to updateStore to clarify purpose Organize and modify Selectors Simplify imports Create index.ts for all actions and reducers Move selectors to features folder Import selectors from new location, and rename soundingSel to pluginSel Remove old selectors Add typing to math.ts Add types to clouds.ts Fix selectAt return type to number Copy arrays for sorting, due to mutation of .sort() Convert to functional component and add types use keys to make JSX valid Prevent error on selection of dropdown default value after selecting a valid fav Add event types and use .currentTarget in place of .target, to accurately access value of the SelectElement Retain class component Include default render function, and props and state typing for PureComponent Return LoadingIndicator to class component, with proper props desctructuring Restrict MetricState types further set number as yPointer type Add flexible types for PureComponent Add types and prevent naming clash Add key for react array optimization and compliance Use function instead of const to prevent 'used before declared' errors Add types to wind.tsx use regular class component destructuring to comply with render method signature
Convert to slices
Reviewer's Guide by SourceryThis pull request introduces significant refactoring and enhancements to the codebase, primarily focusing on the creation of slices for Redux state management, TypeScript type improvements, and various configuration updates. The changes include the addition of new Redux slices, updates to ESLint and Rollup configurations, and the refactoring of several components and utility functions to improve type safety and maintainability. File-Level Changes
Tips
|
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.
Hey @johnmarkredding - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 7 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
'no-import-assign': 'warn', | ||
'a11y-no-static-element-interactions': 'off', | ||
'a11y-missing-attribute': 'off', | ||
"react/react-in-jsx-scope": "off", |
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.
suggestion: Consider adding a comment explaining why this rule is turned off.
Disabling the 'react/react-in-jsx-scope' rule can be confusing for future developers. Adding a comment explaining the rationale behind this decision would be helpful.
"react/react-in-jsx-scope": "off", | |
// Disabling this rule because React 17+ with the new JSX transform | |
// no longer requires React to be in scope. | |
"react/react-in-jsx-scope": "off", |
if (store) { | ||
return store; | ||
import { configureStore, ThunkAction, Action } from '@reduxjs/toolkit'; | ||
import rootReducer, { |
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.
suggestion: Consider renaming the function for clarity.
The function 'updateStore' might be better named as 'initializeStore' or 'configureStore' to better reflect its purpose, which is to set up the store initially.
import rootReducer, { | |
import { configureStore, ThunkAction, Action } from '@reduxjs/toolkit'; | |
import rootReducer, { | |
setWidth, | |
setHeight, | |
} from './rootReducer'; |
import { sampleAt } from "../util/math"; | ||
|
||
export class WindGram extends PureComponent { | ||
constructor(props) { | ||
type WindGramProps = { |
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.
suggestion: Consider adding JSDoc comments for the props.
Adding JSDoc comments for the 'WindGramProps' type can improve code readability and provide better context for future developers.
type WindGramProps = { | |
/** | |
* Props for the WindGram component. | |
* @property {boolean} [isLoading] - Indicates if the component is in a loading state. | |
* @property {ReturnType<typeof skewTSel.params>} [params] - Parameters for the skewTSel function. | |
*/ | |
type WindGramProps = { | |
isLoading?: boolean; | |
params?: ReturnType<typeof skewTSel.params>; | |
}; |
import windyStore from "@windy/store"; | ||
import windyUtils from "@windy/utils"; | ||
import windyModels from "@windy/models"; | ||
|
||
function label(favorite) { | ||
type FavoritesProps = { |
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.
suggestion: Consider adding JSDoc comments for the props.
Adding JSDoc comments for the 'FavoritesProps' type can improve code readability and provide better context for future developers.
type FavoritesProps = { | |
/** | |
* Props for the Favorites component. | |
* @property {PluginState["favorites"]} favorites - List of favorite items. | |
* @property {string} location - Current location. | |
*/ | |
type FavoritesProps = { | |
favorites: PluginState["favorites"]; | |
location: string; | |
}; |
@@ -0,0 +1,177 @@ | |||
import { createSlice, PayloadAction } from "@reduxjs/toolkit"; |
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.
suggestion: Consider splitting the slice into smaller slices.
The 'pluginSlice' is handling a lot of state and actions. Consider splitting it into smaller, more focused slices to improve maintainability and readability.
import { createSlice, PayloadAction } from "@reduxjs/toolkit"; | |
import { createSlice, PayloadAction } from "@reduxjs/toolkit"; | |
import pluginStateSlice from "./pluginStateSlice"; | |
import pluginActionsSlice from "./pluginActionsSlice"; |
@@ -0,0 +1,158 @@ | |||
import windyUtils from "@windy/utils"; |
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.
suggestion: Consider adding a comment explaining the purpose of the slice.
Adding a comment at the top of the file explaining the purpose of the 'modelSlice' can provide better context for future developers.
import windyUtils from "@windy/utils"; | |
/** | |
* This file defines the 'modelSlice' which is responsible for managing the state | |
* related to the model feature in the application. It utilizes utilities, subscriptions, | |
* and products from the Windy library to handle various functionalities. | |
*/ | |
import windyUtils from "@windy/utils"; |
@@ -0,0 +1,6 @@ | |||
import { useDispatch, useSelector } from 'react-redux'; |
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.
suggestion: Consider adding a comment explaining the custom hooks.
Adding a comment explaining the purpose and usage of the custom hooks 'useAppDispatch' and 'useAppSelector' can improve code readability and provide better context for future developers.
import { useDispatch, useSelector } from 'react-redux'; | |
import { useDispatch, useSelector } from 'react-redux'; | |
// Custom hooks to use throughout the app for dispatching actions and selecting state | |
import type { AppDispatch, RootState } from 'src/util/store'; |
@@ -13,11 +14,11 @@ for (let i = 0; i < 160; i++) { | |||
// Output an object: | |||
// - clouds: the clouds cover, | |||
// - width & height: dimension of the cover data. | |||
export function computeClouds(airData) { | |||
export function computeClouds(airData: MeteogramDataPayload) { |
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.
issue (complexity): Consider simplifying the code while maintaining type safety.
The new code introduces additional complexity without significant benefits. Here are some points to consider:
-
Type Annotations: While type annotations can improve code safety, they should be added thoughtfully. In this case, some annotations are redundant (e.g.,
const numX = airData["rh-1000h"].length as number;
). -
Unnecessary Generics: The use of generics in
cubicInterpolate<T extends number>
is unnecessary since all parameters are numbers, adding complexity without benefit. -
Verbose Code: The new code is more verbose, making it harder to read and maintain. Repeated type annotations for arrays (
number[]
) clutter the code. -
New Type Definitions: The introduction of the
CloudCanvas
type adds more lines and complexity. While type definitions can be useful, this seems overkill for a simple function parameter.
Consider simplifying the code while maintaining type safety. Here's a less complex version:
import { lerp } from "./math";
export const hrAlt = [0, 5, 11, 16.7, 25, 33.4, 50, 58.4, 66.7, 75, 83.3, 92, 98, 100];
const hrAltPressure = [null, 950, 925, 900, 850, 800, 700, 600, 500, 400, 300, 200, 150, null];
const lookup = new Uint8Array(256);
for (let i = 0; i < 160; i++) {
lookup[i] = clampIndex(24 * Math.floor((i + 12) / 16), 160);
}
export function computeClouds(airData: { [key: string]: number[] }) {
const numX = airData["rh-1000h"].length;
const numY = hrAltPressure.length;
const rawClouds = new Array(numX * numY);
for (let y = 0, index = 0; y < numY; ++y) {
if (hrAltPressure[y] == null) {
for (let x = 0; x < numX; ++x) {
rawClouds[index++] = 0.0;
}
} else {
const weight = hrAlt[y] * 0.01;
const pAdd = lerp(-60, -70, weight);
const pMul = lerp(0.025, 0.038, weight);
const pPow = lerp(1.0, 1.5, weight);
const pMul2 = lerp(0.5, 0.8, weight);
for (let x = 0; x < numX; ++x) {
let f = airData["rh-1000h"][x] + pAdd;
f = Math.pow(f, pPow) * pMul2;
rawClouds[index++] = f;
}
}
}
const sliceWidth = 10;
const width = sliceWidth * numX;
const height = 300;
const clouds = new Array(width * height);
const kh = (height - 1) * 0.01;
const dx2 = (sliceWidth + 1) >> 1;
let heightLookupIndex = 2 * height;
const heightLookup = new Array(heightLookupIndex);
const buffer = new Array(16);
let previousY;
let currentY = height;
for (let j = 0; j < numY - 1; ++j) {
previousY = currentY;
currentY = Math.round(height - 1 - hrAlt[j + 1] * kh);
const j0 = numX * clampIndex(j + 2, numY);
const j1 = numX * clampIndex(j + 1, numY);
const j2 = numX * clampIndex(j + 0, numY);
const j3 = numX * clampIndex(j - 1, numY);
let previousX = 0;
let currentX = sliceWidth;
for (let i = 0; i < numX; ++i) {
const x0 = previousX;
const x1 = currentX;
const x2 = currentX + dx2;
const x3 = currentX + sliceWidth;
for (let y = previousY; y < currentY; ++y) {
const y0 = heightLookup[heightLookupIndex - y];
const y1 = heightLookup[heightLookupIndex - y - 1];
const y2 = heightLookup[heightLookupIndex - y - 2];
const y3 = heightLookup[heightLookupIndex - y - 3];
const m = buffer;
m[0] = rawClouds[j0 + x0];
m[1] = rawClouds[j0 + x1];
m[2] = rawClouds[j0 + x2];
m[3] = rawClouds[j0 + x3];
m[4] = rawClouds[j1 + x0];
m[5] = rawClouds[j1 + x1];
m[6] = rawClouds[j1 + x2];
m[7] = rawClouds[j1 + x3];
m[8] = rawClouds[j2 + x0];
m[9] = rawClouds[j2 + x1];
m[10] = rawClouds[j2 + x2];
m[11] = rawClouds[j2 + x3];
m[12] = rawClouds[j3 + x0];
m[13] = rawClouds[j3 + x1];
m[14] = rawClouds[j3 + x2];
m[15] = rawClouds[j3 + x3];
clouds[y * width + x0] = bicubicFiltering(m, 0.5, 0.5);
}
}
}
return { clouds, width, height };
}
function clampIndex(index: number, size: number) {
return index < 0 ? 0 : index > size - 1 ? size - 1 : index;
}
function step(x: number) {
return lookup[Math.floor(clampIndex(x, 160))];
}
function cubicInterpolate(y0: number, y1: number, y2: number, y3: number, m: number) {
const a0 = -y0 * 0.5 + 3.0 * y1 * 0.5 - 3.0 * y2 * 0.5 + y3 * 0.5;
const a1 = y0 - 5.0 * y1 * 0.5 + 2.0 * y2 - y3 * 0.5;
const a2 = -y0 * 0.5 + y2 * 0.5;
return a0 * m ** 3 + a1 * m ** 2 + a2 * m + y1;
}
function bicubicFiltering(m: number[], s: number, t: number) {
return cubicInterpolate(
cubicInterpolate(m[0], m[1], m[2], m[3], s),
cubicInterpolate(m[4], m[5], m[6], m[7], s),
cubicInterpolate(m[8], m[9], m[10], m[11], s),
cubicInterpolate(m[12], m[13], m[14], m[15], s),
t
);
}
export function cloudsToCanvas(clouds: number[], width: number, height: number, canvas: HTMLCanvasElement | null) {
if (canvas == null) {
canvas = document.createElement("canvas");
}
canvas.width = width;
canvas.height = height;
const ctx = canvas.getContext("2d");
const imageData = ctx.getImageData(0, 0, width, height);
const imgData = imageData.data;
let srcOffset = 0;
for (let y = 0; y < height; ++y) {
for (let x = 0; x < width; ++x) {
const value = clouds[srcOffset++];
const color = Math.round(value * 255);
const offset = (y * width + x) * 4;
imgData[offset] = color;
imgData[offset + 1] = color;
imgData[offset + 2] = color;
imgData[offset + 3] = 255;
}
}
ctx.putImageData(imageData, 0, 0);
}
This version maintains type safety while being more readable and maintainable.
This is too big of a pull, I'm sure. Let me know how you'd prefer to do this.
I've tried to test each of the affected files as I've made changes. There are a couple things in the math files and selectors that I haven't figured out, so I haven't updated those fully yet.
Summary by Sourcery
This pull request introduces significant refactoring and enhancements to the state management of the application by implementing Redux slices and TypeScript types. It also updates the ESLint and Rollup configurations for improved code quality and build process. Additionally, it removes deprecated Redux actions, reducers, and selectors.