Firefly-2100: Fix ESLint identified errors with behavioral impacts - #2024
Conversation
aventura121
changed the title
aventura121
changed the title
jaladh-singhal
left a comment
jaladh-singhal
left a comment
There was a problem hiding this comment.
Tested most of the items in UI, they are working as expected. Also unit-tests were helpful to understand different cases and GA build passes for them!
Code changes looks good too.
| expr.getParsedVariables().forEach((v) => { | ||
| if (!v.startsWith('"')) { | ||
| const re = new RegExp('([^A-Za-z\d_"]|^)(' + v + ')([^A-Za-z\d_"]|$)', 'g'); | ||
| const re = new RegExp('([^A-Za-z0-9_"]|^)(' + v + ')([^A-Za-z0-9_"]|$)', 'g'); |
There was a problem hiding this comment.
could have changed to \\d but this is fine as well
robyww
left a comment
robyww
left a comment
There was a problem hiding this comment.
Code looks good and there are some real bugs fixed. The improvement to Firefly.js is a cleanup I had wanted to do.
I think no-constant-binary-expression in src/firefly/js/visualize/WebPlot.js:464 is a false positive but no a big deal.
I am glad to see that you started some new test files that can be built on.
| const plotState= PlotState.makePlotStateWithJson(wpInit.plotState,request0, rv0); | ||
| if (!request0) request0= plotState.getWebPlotRequest(); | ||
| const colorTableId= request0?.getInitialColorTable()+'' ?? '0'; | ||
| const colorTableId= String(request0?.getInitialColorTable() ?? '0'); |
There was a problem hiding this comment.
This is fine but I think request0?.getInitialColorTable() will always return a number and never an undefined or NaN so the the original code is valid.
Also, I don't think ESlint was returning an issue here.
| * @returns {Promise.<void>} | ||
| */ | ||
| function bootstrap(props, clientAppSpecificOptions, webApiCommands) { | ||
| async function bootstrap(props, clientAppSpecificOptions, webApiCommands) { |
There was a problem hiding this comment.
this is a good change. Much cleaner.
|
|
||
| client.addListener(ActionEventHandler); | ||
| window.firefly.wsClient = client; | ||
| notifyServerAppInit({spaName:`${props.appTitle||''}--${props.template?props.template:'api'}`}); |
There was a problem hiding this comment.
since we are cleaning up, this probably should be
void notifyServerAppInit({spaName:`${props.appTitle||''}--${props.template?props.template:'api'}`});to indicate we don't care about the promise.
| defer(() => { | ||
| setTimeout(() => { | ||
| dispatchUpdateAppData({isReady: true}); | ||
| },3); |
There was a problem hiding this comment.
Now that I look tat this code, I wonder why we need a double defer. Probably just a setTimeout(f,5) would be fine but I think that is outside of the scope of this PR.
e5b6d0c to
bb2163e
Compare
3 participants
Firefly-2100: "Triage (or Fix) ESLint-Identified Javascript/React Issues"
Description
This PR fixes ESLint identified errors with potential behavioral impacts.
Except for
react-hooks/rules-of-hooks, these rules come fromjs.configs.recommended, which our ESLint config doesn't yet enable. I've included the command that reproduces its error ondevbelow; on this branch the same command should not report the specific error. The commands are run from the repo root.Builds
Firefly: https://firefly-2100-eslint-identified-bugs-clean-2.irsakubedev.ipac.caltech.edu/firefly
Finder Chart: https://firefly-2100-eslint-identified-bugs-clean.irsakubedev.ipac.caltech.edu/applications/finderchart
Irsa Viewer: https://firefly-2100-eslint-identified-bugs-clean.irsakubedev.ipac.caltech.edu/irsaviewer
SphereX: https://firefly-2100-eslint-identified-bugs-clean.irsakubedev.ipac.caltech.edu/applications/spherex
Testing
1.
no-async-promise-executorinsrc/firefly/js/Firefly.js:444npx eslint --rule 'no-async-promise-executor: error' src/firefly/js/Firefly.jsbootstrap()is nowasyncinstead.2.
no-useless-escapeinsrc/firefly/js/charts/ChartUtil.js:1198npx eslint --rule 'no-useless-escape: error' src/firefly/js/charts/ChartUtil.js\dresolves to the letterd. Change\dto0-9.src/firefly/js/charts/__tests__/ChartUtil-test.js:formatColExpr does not treat a digit as the edge of a column namenpx jest --config src/firefly/jest.config.js src/firefly/js/charts/__tests__/ChartUtil-test.js3.
no-fallthroughinsrc/firefly/js/util/expr/Expr.js:234npx eslint --rule 'no-fallthrough: error' src/firefly/js/util/expr/Expr.jsreturntocase NVL2.src/firefly/js/util/__tests__/Expression-test.js:nvl2npx jest --config src/firefly/jest.config.js src/firefly/js/util/__tests__/Expression-test.js -t nvl24.
no-unsafe-optional-chaininginsrc/firefly/js/tables/HpxIndexCntlr.js:801, 810npx eslint --rule 'no-unsafe-optional-chaining: error' src/firefly/js/tables/HpxIndexCntlr.js[]before spreading, and added the!orderDataguard thatgetKeysForOrderlacked.src/firefly/js/tables/__tests__/HpxIndexCntlr-test.js:getValuesForOrder and getKeysForOrder return the tile entries of an ordergetValuesForOrder and getKeysForOrder return an empty array for a missing ordergetKeysForOrder returns an empty array without orderDatanpx jest --config src/firefly/jest.config.js src/firefly/js/tables/__tests__/HpxIndexCntlr-test.js5.
react-hooks/rules-of-hooksinsrc/firefly/js/charts/ui/CombineChart.jsx:119npx eslint src/firefly/js/charts/ui/CombineChart.jsx(this rule is already on)return nullbelow theuseStoreConnectorhook.CombinePinnedChartsinto the DOM and see it renders:Screenshot
6.
no-constant-binary-expressioninsrc/firefly/js/visualize/WebPlot.js:464npx eslint --rule 'no-constant-binary-expression: error' src/firefly/js/visualize/WebPlot.js?? '0'before converting to a string, so a missing plot request gives'0'.Screenshot WebPlot
7.
no-dupe-else-ifinsrc/firefly/js/visualize/draw/SelectBox.js:201npx eslint --rule 'no-dupe-else-if: error' src/firefly/js/visualize/draw/SelectBox.jscirclecheck is nowellipse.8.
no-cond-assigninsrc/firefly/js/util/ObsCoreSRegionParser.js:203npx eslint --rule 'no-cond-assign: error' src/firefly/js/util/ObsCoreSRegionParser.js=→===in the BOX length check. Also passUnitType.ARCSECinstead of the string'arcsec', sincegetDimreadstoUnit.key.s_regionis not used in the app yet, as far as I could find).