Nickakhmetov/HMP-194 Apply fixes from lint config #3143

NickAkhmetov · 2023-06-28T17:39:17Z

I organized the commits by the rules they pertained to where appropriate; some edits were made to the configs as well (which I'll be documenting)

This PR is a draft for now as I'm still in the process of getting tests to pass; I will request reviews as soon as that's done. In the meantime, the changes I've made so far are now visible.

converted components to use `function` adjusts jest tests to use `not.toBeInTheDocument` instead of `toBeNull` removes unnecessary fragments removes trailing spaces

Do not define components during render. React will see a new component type on every render and destroy the entire subtree’s DOM nodes and state (https://reactjs.org/docs/reconciliation.html#elements-of-different-types). Instead, move this component definition out of the parent component “VerticalStackedBarChart” and pass data as props.

The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook I also turned the functions in the Files component into callbacks since they're provided in the context

… expects in tests

…defined inside of `ProvGraph`

…g-assignment`)

…ack` with unknown dependencies

The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook

…xt` instead

…defined, fix optional chain safety

…keys

…resent `testing-library/prefer-presence-queries`

…dependencies

…run successfully

…-194-lint-fixes

john-conroy

Thankfully most of it was swapping functional components from arrow functions to the traditional function syntax. Were all of these changes prompted by eslint? It seems like there were a few changes that might not have been and it's a little difficult for me to discern.

context/app/static/js/components/detailPage/files/FileBrowser/FileBrowser.spec.js

context/app/static/js/components/entity-search/SearchWrapper/utils.spec.js

john-conroy · 2023-06-29T18:52:03Z

context/app/static/js/shared-styles/buttons/index.js

+  children,
+  tooltipComponent,
+  tooltipTitle,
+  buttonComponent: _buttonComponent,


Seems like an unused prop that should be removed?

Same as in the other case; if this is ever called with a buttonComponent prop defined, it would be implicitly propagated to WhiteBackgroundToggleButton, so I marked it as unused with the underscore prefix to be safe.

john-conroy · 2023-06-29T18:54:05Z

context/app/static/js/components/entity-search/SearchWrapper/utils.js

@@ -42,7 +42,7 @@ function buildFieldConfig({
  type,
  facetGroup = 'Additional Facets',
  configureGroup = 'General',
-  entityType: pageEntityType,
+  entityType: _pageEntityType,


Also unused?

Since this prop is part of a spread deconstruction operator, removing it altogether would implicitly add entityType to the rest object whenever entityType is ever provided as part of the input object. This would lead to the entityType key being propagated into the field config returned since ...rest is part of the returned fields, and may cause unforeseen bugs to pop up. My understanding of this key being present in the deconstructed props but unused otherwise is that entityType is being purposely excluded from the returned object (assuming that the related search functionality is working as expected)

john-conroy · 2023-06-29T19:12:54Z

context/build-utils/webpack.common.js

    fallback: {
      // Now necessary because webpack 5 doesn't include these polyfills by default
      timers: require.resolve('timers-browserify'),
      stream: require.resolve('stream-browserify'),
+      buffer: require.resolve('buffer'),


What required introducing the polyfill?

Same xml2js library as before started throwing errors during docker build until I included this. I added this to the related ticket so it's removed alongside the other ones.

john-conroy · 2023-06-29T19:15:54Z

context/app/static/js/shared-styles/charts/VerticalStackedBarChart/VerticalStackedBarChart.jsx

+function TickComponentWithHandlers({ handleMouseEnter, handleMouseLeave }) {
+  // use a callback to avoid creating a new component on every render
+  return useCallback(
+    ({ formattedValue, ...tickProps }) => {
+      return (
+        <Text onMouseEnter={handleMouseEnter({ key: formattedValue })} onMouseLeave={handleMouseLeave} {...tickProps}>
+          {trimStringWithMiddleEllipsis(formattedValue)}
+        </Text>
+      );
+    },
+    [handleMouseEnter, handleMouseLeave],
+  );
+}
+


This pattern seems strange, was this the outcome of an eslint suggestion? Which rule?

The main issue that eslint was complaining about was react/no-unstable-nested-elements: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md

A new TickComponent component was being created and torn down on each render of the VerticalStackedBarChart since it was previously an anonymous component being passed down as props.

Just pulling it out into a TickComponent by itself and trying to pass that in as a ComponentType didn't work since it uses the handleMouseEnter/handleMouseLeave functions that are not part of the interface of the props that are provided to the tickComponent

As a result, I made this HOC-like component. My thought process is that useCallback should ensure referential equality between instances of the TickComponent that have stable handleMouseEnter/handleMouseLeave and therefore minimize VDOM churn (essentially using useCallback to generate a pure component with the current mouse handler functions)

One alternative approach would be putting the handleMouseEnter/handleMouseLeave functions into a context so that TickComponent only needs the props provided by AxisBottom and the functions are accessed via useContext.

I also considered just dropping the useCallback and making it a normal HOC like (mouseHandlers) => (tickProps) => JSX, but that looks a lot like the exact unstable-nested-components pattern that I'm trying to work around 😅

Thanks for the explanation. Let's keep it how it is here then.

context/app/static/js/components/detailPage/provenance/ProvVis/__tests__/ProvVis.spec.js

john-conroy · 2023-06-29T19:20:24Z

context/app/static/js/components/detailPage/provenance/ProvVis/ProvVis.jsx

@@ -15,7 +15,7 @@ export default function ProvVis({ provData, getNameForActivity, getNameForEntity
  const { steps, setSteps } = useProvenanceStore(useProvenanceStoreSelector);

  useEffect(() => {
-    setSteps(new ProvData(provData, getNameForActivity, getNameForEntity, entity_type).toCwl());
+    setSteps(new ProvData(provData, entity_type, getNameForActivity, getNameForEntity).toCwl());


What prompted the argument reordering?

https://eslint.org/docs/latest/rules/default-param-last

Since ProvData has default arguments defined for getNameForActivity and getNameForEntity, the convention is to put them last so they can be omitted entirely; otherwise, there's no way to define entity_type without also providing values for the other two arguments.

Alternatively - we could change the constructor of ProvData to take in an object with these args instead of having them be positional (i.e. so the constructor is new ProvData({provData, entity_type, getNameForActivity, getNameForEntity}), we can have any of them be optional without worrying about their order; we'd still need to modify all the calls to new ProvData accordingly, but this idea seems a bit more maintainable in the long run.

Preference to use an object, but shouldn't block here.

I've made this adjustment, I prefer that alternative approach as well. The only downside is needing to make sure that the prov prop is actually being passed in as prov (which I've adjusted accordingly where needed)

context/app/static/js/components/detailPage/provenance/ProvGraph/ProvGraph.jsx

…tils.spec.js Co-authored-by: John Conroy <[email protected]>

…-194-lint-fixes

…3142) * Ignore `settings.json`, add `default.settings.json` * bump linting-related dependencies, add typescrip-eslint deps, remove babel * WIP - adding typescript, updating eslint config * Expand eslint/prettier config * Get tsconfig working, adjust swcrc config to allow typescript * target more file types with eslint formatter * allow webpack files to import devDependencies * Apply webpack overrides to entire build-utils folder to cover alias.js as well * add paths to tsconfig, add optional config to default settings, add autopep8 to recommended extensions * No need to limit to only `node` types * remove babel dependencies * added changelog * propagate changes to `.eslintrc.yml` from fixes branch * Update CHANGELOG-lint-updates.md Co-authored-by: John Conroy <[email protected]> * Nickakhmetov/HMP-194 Apply fixes from lint config (#3143) * `npm run lint:fix` autofixes only converted components to use `function` adjusts jest tests to use `not.toBeInTheDocument` instead of `toBeNull` removes unnecessary fragments removes trailing spaces * Fix `react/no-unstable-nested-components` error Do not define components during render. React will see a new component type on every render and destroy the entire subtree’s DOM nodes and state (https://reactjs.org/docs/reconciliation.html#elements-of-different-types). Instead, move this component definition out of the parent component “VerticalStackedBarChart” and pass data as props. * allow use of require in jest tests * fix resolution of `fonts.ts` file asking for the extension * Fix `react/jsx-no-constructed-context-values` errors The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook I also turned the functions in the Files component into callbacks since they're provided in the context * Allow helpers that contain `expect` statements to be used as the only expects in tests * add tsconfig to dockerfile * Fix `react/no-unstable-nested-components` with `DetailPanel` getting defined inside of `ProvGraph` * Fix undefined `test` global in test fixtures * Disable `jsx-no-bind` rule * Must use destructuring `themeContext` assignment (`react/destructuring-assignment`) * `Default parameters should be last` * Reorganized debounced vitessce config setter to avoid using `useCallback` with unknown dependencies * Name HOC components (`func-names`) * fix `no-unused-vars`; allow unused vars if they start with `_` prefix * use destructuring props assignment * `react/jsx-no-constructed-context-values` The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook * allow empty function placeholders in test files * add assertions to accordion list filter tests * Avoid destructuring queries from `render` result, use `screen.getByText` instead * another "screen" fix * allow "redundant" story names pending further investigation * reorganize to make sure components are only referenced after they're defined, fix optional chain safety * uncomment test for the time being, will troubleshoot * prepend unused var with `_` * fix describe block name * remove `as default`, make it an explicit default export * allow variables to remain unused if spreading object to remove those keys * Add explicit explanation for empty effect cleanup function * Use `getBy*` queries rather than `queryBy*` for checking element is present `testing-library/prefer-presence-queries` * explicitly use default export instead of `as default` * use `useMemo` to manage SearchKitManager instance to fix `useEffect` dependencies * add swc helpers package to dev dependencies to make jest test suites run successfully * Fix ProvVis test * fix accordion filter tests * Add `buffer` polyfill to make docker image build correctly * Fix maintenance cypress test * remove unnecessary usecallbacks * Update context/app/static/js/components/entity-search/SearchWrapper/utils.spec.js Co-authored-by: John Conroy <[email protected]> * Fix Jest test-utils resolution * remove unnecessary import * Change ProvData constructor to an object --------- Co-authored-by: John Conroy <[email protected]> --------- Co-authored-by: John Conroy <[email protected]>

* Ignore `settings.json`, add `default.settings.json` * bump linting-related dependencies, add typescrip-eslint deps, remove babel * WIP - adding typescript, updating eslint config * Expand eslint/prettier config * Get tsconfig working, adjust swcrc config to allow typescript * target more file types with eslint formatter * allow webpack files to import devDependencies * Apply webpack overrides to entire build-utils folder to cover alias.js as well * add paths to tsconfig, add optional config to default settings, add autopep8 to recommended extensions * No need to limit to only `node` types * remove babel dependencies * added changelog * `npm run lint:fix` autofixes only converted components to use `function` adjusts jest tests to use `not.toBeInTheDocument` instead of `toBeNull` removes unnecessary fragments removes trailing spaces * Fix `react/no-unstable-nested-components` error Do not define components during render. React will see a new component type on every render and destroy the entire subtree’s DOM nodes and state (https://reactjs.org/docs/reconciliation.html#elements-of-different-types). Instead, move this component definition out of the parent component “VerticalStackedBarChart” and pass data as props. * allow use of require in jest tests * fix resolution of `fonts.ts` file asking for the extension * Fix `react/jsx-no-constructed-context-values` errors The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook I also turned the functions in the Files component into callbacks since they're provided in the context * Allow helpers that contain `expect` statements to be used as the only expects in tests * add tsconfig to dockerfile * Fix `react/no-unstable-nested-components` with `DetailPanel` getting defined inside of `ProvGraph` * Fix undefined `test` global in test fixtures * Disable `jsx-no-bind` rule * Must use destructuring `themeContext` assignment (`react/destructuring-assignment`) * `Default parameters should be last` * Reorganized debounced vitessce config setter to avoid using `useCallback` with unknown dependencies * Name HOC components (`func-names`) * fix `no-unused-vars`; allow unused vars if they start with `_` prefix * use destructuring props assignment * `react/jsx-no-constructed-context-values` The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook * allow empty function placeholders in test files * add assertions to accordion list filter tests * Avoid destructuring queries from `render` result, use `screen.getByText` instead * another "screen" fix * allow "redundant" story names pending further investigation * reorganize to make sure components are only referenced after they're defined, fix optional chain safety * uncomment test for the time being, will troubleshoot * prepend unused var with `_` * fix describe block name * remove `as default`, make it an explicit default export * allow variables to remain unused if spreading object to remove those keys * Add explicit explanation for empty effect cleanup function * Use `getBy*` queries rather than `queryBy*` for checking element is present `testing-library/prefer-presence-queries` * explicitly use default export instead of `as default` * use `useMemo` to manage SearchKitManager instance to fix `useEffect` dependencies * add swc helpers package to dev dependencies to make jest test suites run successfully * Fix ProvVis test * fix accordion filter tests * Add `buffer` polyfill to make docker image build correctly * Fix maintenance cypress test * rework existing tests to work with vitessce publication * remove waits from tests, add tests for authors/provenance sections * test vignette accordion * Add changelog * propagate changes to `.eslintrc.yml` from fixes branch * remove unnecessary usecallbacks * Update context/app/static/js/components/entity-search/SearchWrapper/utils.spec.js Co-authored-by: John Conroy <[email protected]> * Fix Jest test-utils resolution * remove unnecessary import * Update CHANGELOG-lint-updates.md Co-authored-by: John Conroy <[email protected]> * Change ProvData constructor to an object * Nickakhmetov/HMP-194 Apply fixes from lint config (#3143) * `npm run lint:fix` autofixes only converted components to use `function` adjusts jest tests to use `not.toBeInTheDocument` instead of `toBeNull` removes unnecessary fragments removes trailing spaces * Fix `react/no-unstable-nested-components` error Do not define components during render. React will see a new component type on every render and destroy the entire subtree’s DOM nodes and state (https://reactjs.org/docs/reconciliation.html#elements-of-different-types). Instead, move this component definition out of the parent component “VerticalStackedBarChart” and pass data as props. * allow use of require in jest tests * fix resolution of `fonts.ts` file asking for the extension * Fix `react/jsx-no-constructed-context-values` errors The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook I also turned the functions in the Files component into callbacks since they're provided in the context * Allow helpers that contain `expect` statements to be used as the only expects in tests * add tsconfig to dockerfile * Fix `react/no-unstable-nested-components` with `DetailPanel` getting defined inside of `ProvGraph` * Fix undefined `test` global in test fixtures * Disable `jsx-no-bind` rule * Must use destructuring `themeContext` assignment (`react/destructuring-assignment`) * `Default parameters should be last` * Reorganized debounced vitessce config setter to avoid using `useCallback` with unknown dependencies * Name HOC components (`func-names`) * fix `no-unused-vars`; allow unused vars if they start with `_` prefix * use destructuring props assignment * `react/jsx-no-constructed-context-values` The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook * allow empty function placeholders in test files * add assertions to accordion list filter tests * Avoid destructuring queries from `render` result, use `screen.getByText` instead * another "screen" fix * allow "redundant" story names pending further investigation * reorganize to make sure components are only referenced after they're defined, fix optional chain safety * uncomment test for the time being, will troubleshoot * prepend unused var with `_` * fix describe block name * remove `as default`, make it an explicit default export * allow variables to remain unused if spreading object to remove those keys * Add explicit explanation for empty effect cleanup function * Use `getBy*` queries rather than `queryBy*` for checking element is present `testing-library/prefer-presence-queries` * explicitly use default export instead of `as default` * use `useMemo` to manage SearchKitManager instance to fix `useEffect` dependencies * add swc helpers package to dev dependencies to make jest test suites run successfully * Fix ProvVis test * fix accordion filter tests * Add `buffer` polyfill to make docker image build correctly * Fix maintenance cypress test * remove unnecessary usecallbacks * Update context/app/static/js/components/entity-search/SearchWrapper/utils.spec.js Co-authored-by: John Conroy <[email protected]> * Fix Jest test-utils resolution * remove unnecessary import * Change ProvData constructor to an object --------- Co-authored-by: John Conroy <[email protected]> * remove ESLint changelog that propagated from a merge * Remove `table` from provenance section tests (publications don't have prov tables) * Try waiting before searching for entity header? * force click for vignette test --------- Co-authored-by: John Conroy <[email protected]>

NickAkhmetov added 30 commits June 27, 2023 15:34

npm run lint:fix autofixes only

1f2dc00

converted components to use `function` adjusts jest tests to use `not.toBeInTheDocument` instead of `toBeNull` removes unnecessary fragments removes trailing spaces

allow use of require in jest tests

e88b8cc

fix resolution of fonts.ts file asking for the extension

b9fb281

Allow helpers that contain expect statements to be used as the only…

e70f38e

… expects in tests

add tsconfig to dockerfile

b6ad5ea

Fix react/no-unstable-nested-components with DetailPanel getting …

a939c7d

…defined inside of `ProvGraph`

Fix undefined test global in test fixtures

189cf51

Disable jsx-no-bind rule

6f5c2d6

Must use destructuring themeContext assignment (`react/destructurin…

adb0f0b

…g-assignment`)

Default parameters should be last

2bf9771

Reorganized debounced vitessce config setter to avoid using `useCallb…

3cdd8a4

…ack` with unknown dependencies

Name HOC components (func-names)

9572b0c

fix no-unused-vars; allow unused vars if they start with _ prefix

d1c2280

use destructuring props assignment

72ca7f7

react/jsx-no-constructed-context-values

4463fca

The object passed as the value prop to the Context provider changes every render. To fix this consider wrapping it in a useMemo hook

allow empty function placeholders in test files

44c6a77

add assertions to accordion list filter tests

0a5402a

Avoid destructuring queries from render result, use `screen.getByTe…

653b8aa

…xt` instead

another "screen" fix

630c50a

allow "redundant" story names pending further investigation

7938dd4

reorganize to make sure components are only referenced after they're …

a50b498

…defined, fix optional chain safety

uncomment test for the time being, will troubleshoot

a0b57ff

prepend unused var with _

f4db659

fix describe block name

36856b2

remove as default, make it an explicit default export

6424105

allow variables to remain unused if spreading object to remove those …

0ca12d9

…keys

Add explicit explanation for empty effect cleanup function

778bd2c

Use getBy* queries rather than queryBy* for checking element is p…

1ec2929

…resent `testing-library/prefer-presence-queries`

NickAkhmetov added 7 commits June 28, 2023 13:09

explicitly use default export instead of as default

65ed652

use useMemo to manage SearchKitManager instance to fix useEffect …

989fc08

…dependencies

add swc helpers package to dev dependencies to make jest test suites …

4933ef1

…run successfully

Fix ProvVis test

2254352

fix accordion filter tests

d29339f

Add buffer polyfill to make docker image build correctly

a772eb2

Fix maintenance cypress test

88497f5

NickAkhmetov marked this pull request as ready for review June 28, 2023 19:10

NickAkhmetov requested review from lchoy, kevinyooky and john-conroy June 28, 2023 19:10

Merge branch 'nickakhmetov/hmp-194-lint-config' into nickakhmetov/hmp…

617a402

…-194-lint-fixes

john-conroy reviewed Jun 29, 2023

View reviewed changes

NickAkhmetov and others added 5 commits June 30, 2023 10:39

remove unnecessary usecallbacks

fb70724

Update context/app/static/js/components/entity-search/SearchWrapper/u…

af27648

…tils.spec.js Co-authored-by: John Conroy <[email protected]>

Fix Jest test-utils resolution

fd177b7

remove unnecessary import

02a4743

Change ProvData constructor to an object

639673a

john-conroy self-requested a review July 5, 2023 19:23

john-conroy approved these changes Jul 5, 2023

View reviewed changes

Merge branch 'nickakhmetov/hmp-194-lint-config' into nickakhmetov/hmp…

f7f6779

…-194-lint-fixes

NickAkhmetov merged commit e92863f into nickakhmetov/hmp-194-lint-config Jul 5, 2023
5 checks passed

NickAkhmetov deleted the nickakhmetov/hmp-194-lint-fixes branch July 5, 2023 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nickakhmetov/HMP-194 Apply fixes from lint config #3143

Nickakhmetov/HMP-194 Apply fixes from lint config #3143

NickAkhmetov commented Jun 28, 2023

john-conroy left a comment

john-conroy Jun 29, 2023 •

edited

Loading

NickAkhmetov Jun 30, 2023

john-conroy Jun 29, 2023

NickAkhmetov Jun 30, 2023

john-conroy Jun 29, 2023

NickAkhmetov Jun 30, 2023

john-conroy Jun 29, 2023

NickAkhmetov Jun 30, 2023

john-conroy Jul 5, 2023

john-conroy Jun 29, 2023

NickAkhmetov Jun 30, 2023

NickAkhmetov Jun 30, 2023

john-conroy Jul 5, 2023

NickAkhmetov Jul 5, 2023

Nickakhmetov/HMP-194 Apply fixes from lint config #3143

Nickakhmetov/HMP-194 Apply fixes from lint config #3143

Conversation

NickAkhmetov commented Jun 28, 2023

john-conroy left a comment

Choose a reason for hiding this comment

john-conroy Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-conroy Jun 29, 2023 •

edited

Loading