Update re-frame #3269

aatkin · 2024-03-04T15:02:34Z

relates #3105 (front-end update)

After update (day8/re-frame#740), re-frame warns about subscriptions created outside of reactive context.

Document title did not seem to set correctly before, which should be fixed and has browser tests now.

updated package-lock.json to lockfileVersion 3, and package.json dependencies are pinned by exact version
- NB: follow-redirects is downgraded to 1.15.4 (updated by dependabot in chore(deps-dev): bump follow-redirects from 1.15.4 to 1.15.6 #3274). npm ls follow-redirects traces back to karma test runner. edit: follow-redirects is now pinned to 1.15.6 in dev dependencies instead to keep dependabot changes.
updated re-frame from 1.2.0 → 1.4.3 and fixed several warnings "re-frame: Subscribe was called outside of a reactive context":
- rems.dropdown: label functions that call e.g. (rems.text/text ...)
- ~~rems.table: subscription functions that call e.g. (rems.text/text ...)~~ (edit: will be changed later)
- rems.flash-message: use reagent.core/with-let instead of reagent.core/create-class, due to issues with reactive context
- rems.atoms/document-title: use reagent.core/with-let instead of reagent.core/create-class, due to issues with reactive context
- user focus "grab" and lazy-load-data in rems.spa are implemented using reagent.core/track!. at first this was only for focus grab which was splitted from main-content that used reagent.core/create-class, but the pattern seemed to fit hook-like helpers
rems main app is now rendered using rems.spa/app, that separates some setup code from page layout
extracted rems.theme namespace (cljs)
added react strict mode to development build. this has subtle effect of "double rendering", but should help catching issues earlier.

Checklist for author

Remove items that aren't applicable, check items that are done.

Reviewability

Link to issue

Testing

Complex logic is unit tested
Valuable features are integration / browser / acceptance tested automatically

Macroz

Looks pretty good to me. I think we need to think a little bit if we should still decide not to do the raw reaction. Stuff just used to work and the workaround is pretty awful. There seems to be a fix to re-frame in the pipe although in danger of being forgotten.

Maybe it would make sense to do the "raw global var" instead for translations. The mechanism is very central to REMS but using it through the subscriptions is not necessary. If we want to try that, we could do everything here except using the raw reactions, and keep them as is even with the warnings.

package.json

test/clj/rems/test_browser.clj

src/cljs/rems/administration/create_catalogue_item.cljs

Macroz · 2024-04-11T09:54:15Z

src/cljs/rems/administration/create_resource.cljs

-       :items @(rf/subscribe [::duo-codes])
+       :items (vec (for [duo @(rf/subscribe [::duo-codes])
+                         :let [shorthand (:shorthand duo)
+                               label (localized (:label duo))]]
+                     (assoc duo
+                            ::label (text-format :t.label/dash shorthand label))))


Actually, it could be nice to extract some or all of these as subscriptions. It is "further away" from this location, but it would make the dropdown declaration shorter, and perhaps be more performant.

Macroz · 2024-04-11T09:56:45Z

src/cljs/rems/administration/duo.cljs

+           ;; due to async loading, item label cannot be set ahead of time, but label function also cannot use subscriptions.
+           ;; TODO: investigate solutions


I guess here it would help for example, if the localizations were not from a subscription, but e.g. the global var set in the page load.

Could the backend do the localization for the search results?

src/cljs/rems/administration/catalogue_items.cljs

src/cljs/rems/flash_message.cljs

src/cljs/rems/spa.cljs

Macroz · 2024-04-11T10:39:12Z

src/cljs/rems/spa.cljs

+  (r/with-let [tracked (mapv r/track! [lazy-load-data! grab-focus!])]
+
+    (if (config/dev-environment?)
+      [react-strict-mode [page]]


Are there a lot of warnings? Should browser tests fail if there are any/more?

Would actually be nice to fail any browser tests with any seen warnings (or maybe ignore some difficult to fix ones).

Almost none actually, i have only seen one from komponentit.autosize/textarea for using (deprecated) react-dom/findDOMNode instead of ref. react-transition-group (not in this PR) also triggers that warning under strict mode if DOM element ref is not manually handled.

It could be good to test what warnings/how many are produced. #2368 already exists for tracking this, too.

React 18 slightly changes component mounting/unmounting in strict mode: https://react.dev/blog/2022/03/08/react-18-upgrade-guide#updates-to-strict-mode

Although this looks more related to using react/useState, which our components are not using (dropdown via react-select might be).

- updated package-lock.json to v3 (used by npm v9 and above, shipped with Node v20) - pinned node dependencies with exact version - added follow-redirects to package.json - originally security update introduced by dependabot, added here to ensure it is not accidentally wiped out during dependency update

Update to re-frame brought attention to several subscription leaks in rendering. Document title was left untested, probably because previous implementation of document title component did not correctly set the document title. Login page and extra pages may still use different h1, but at least automatic checks are now in place.

As part of re-frame update, subscriptions should no longer be created outside of reactive context. Dropdown constantly re-renders the list of items, and label function is effectively called for each item.

As part of re-frame update, prevent subscription from being created outside of the reactive context. Lifecycle events in class components seem to be a rather common source of these bugs. Reagent has utils for working with these events in functional-y manner, but additional API could be less confusing since mount/update/unmount are more standard than with-let.

- Extract rems.spa/app from page. This allows app to separate lazy-loading/grab-focus "hooks" from page, and render additional React strict mode in dev without entangling layout - Use functional component in rems.spa/app over React class to prevent leaking subscriptions. React class wrappers using subscription seem to be a problematic pattern in this regard - rems.theme namespace. Not much for now, but should encapsulate some special rules like rendering logo only if navbar logo does not exist - React helpers in util namespace. Thin wrappers, but make it slightly easier to use e.g. react-profiler

User might want to see changes reflected to current scroll position when changing language, so forcing focus does not make sense.

src/cljs/rems/theme.cljs

Macroz

Ok let's get this merged and we can find any problems with manual testing.

aatkin force-pushed the update-reframe branch 6 times, most recently from b1d700f to 2649dac Compare March 14, 2024 14:53

aatkin force-pushed the update-reframe branch 2 times, most recently from f2ea32b to cb3798f Compare April 4, 2024 11:32

aatkin marked this pull request as ready for review April 8, 2024 06:45

aatkin changed the title ~~WIP: update re-frame~~ Update re-frame Apr 8, 2024

aatkin force-pushed the update-reframe branch from cae0351 to 8d08705 Compare April 8, 2024 13:56

Macroz reviewed Apr 11, 2024

View reviewed changes

aatkin added 10 commits April 18, 2024 15:06

fix: localize dropdown options within reactive context

446f4df

As part of re-frame update, subscriptions should no longer be created outside of reactive context. Dropdown constantly re-renders the list of items, and label function is effectively called for each item.

fix: dont trigger focus grab when changing language

e20fcdb

User might want to see changes reflected to current scroll position when changing language, so forcing focus does not make sense.

refactor: add explicit else to cond

aee3fa4

test: use wait-predicate over test assertion for page title

d4f3c05

chore: remove unnecessary comment

3885984

fix: use new test helper for waiting page title

8bdb8f8

aatkin force-pushed the update-reframe branch from 68c2349 to 8bdb8f8 Compare April 18, 2024 12:32

Macroz reviewed Apr 23, 2024

View reviewed changes

src/cljs/rems/theme.cljs Outdated Show resolved Hide resolved

Macroz approved these changes Apr 23, 2024

View reviewed changes

aatkin added 5 commits May 16, 2024 15:52

Merge remote-tracking branch 'origin/master' into update-reframe

c8bb9a5

refactor: prefer ns qualified keyword in namespace

6582e8e

chore: remove unused code

54a7313

Merge remote-tracking branch 'origin/master' into update-reframe

f3f6063

chore: remove unused code

482d714

aatkin merged commit 2060ace into master May 17, 2024
7 checks passed

aatkin deleted the update-reframe branch May 17, 2024 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update re-frame #3269

Update re-frame #3269

aatkin commented Mar 4, 2024 •

edited

Loading

Macroz left a comment

Macroz Apr 11, 2024

Macroz Apr 11, 2024

Macroz Apr 23, 2024

Macroz Apr 11, 2024

aatkin Apr 15, 2024

aatkin Apr 15, 2024

Macroz left a comment

		;; due to async loading, item label cannot be set ahead of time, but label function also cannot use subscriptions.
		;; TODO: investigate solutions

Update re-frame #3269

Update re-frame #3269

Conversation

aatkin commented Mar 4, 2024 • edited Loading

Checklist for author

Reviewability

Testing

Macroz left a comment

Choose a reason for hiding this comment

Macroz Apr 11, 2024

Choose a reason for hiding this comment

Macroz Apr 11, 2024

Choose a reason for hiding this comment

Macroz Apr 23, 2024

Choose a reason for hiding this comment

Macroz Apr 11, 2024

Choose a reason for hiding this comment

aatkin Apr 15, 2024

Choose a reason for hiding this comment

aatkin Apr 15, 2024

Choose a reason for hiding this comment

Macroz left a comment

Choose a reason for hiding this comment

aatkin commented Mar 4, 2024 •

edited

Loading