Fix JS errors that only require a check for truthy value #1881

dsevillamartin · 2019-09-09T21:16:13Z

This should fix a bunch of errors that have been thrown recently on our beta demo.

Fixes:

TypeError: Cannot read property 'top' of undefined
  at apply(webpack://@flarum/core/./src/common/components/Dropdown.js:69:15)

TypeError: Cannot read property 'content' of null
  at apply(webpack://@flarum/core/./src/forum/components/ReplyPlaceholder.js:55:46)

TypeError: null is not an object (evaluating 'app.composer.component.content')
  at apply(webpack://@flarum/core/./src/forum/components/ReplyPlaceholder.js:70:30)

TypeError: undefined is not an object (evaluating '.offset().top')
  at anchorScroll(webpack://@flarum/core/./src/common/utils/anchorScroll.js:16:37)
  at onSuccess(webpack://@flarum/core/./src/forum/components/PostStream.js:115:17)
  ...
  at document(webpack://@flarum/core/./src/forum/components/PostStream.js:66:26)

TypeError: Cannot read property 'addDiscussion' of undefined
  at onSuccess(webpack://@flarum/core/./src/forum/components/DiscussionComposer.js:95:34)

franzliedke

I commented on what I saw. Let's not rush this, though - and focus on the remaining planned items for tomorrow's feature freeze.

franzliedke · 2019-09-09T21:41:34Z

js/src/forum/components/DiscussionComposer.js

@@ -92,7 +92,9 @@ export default class DiscussionComposer extends ComposerBody {
    app.store.createRecord('discussions').save(data).then(
      discussion => {
        app.composer.hide();
-        app.cache.discussionList.addDiscussion(discussion);
+
+        if (app.cache.discussionList) app.cache.discussionList.addDiscussion(discussion);


This is likely made obsolete by #1868.

That PR would still need a check anyway... I'll remove the extra lines I added so maybe git just sees it as adding the if before, and not changing the whole line.

franzliedke · 2019-09-09T21:42:39Z

js/src/common/utils/anchorScroll.js

@@ -12,10 +12,18 @@
 * @param {Function} callback The callback to run that will change page content.
 */
 export default function anchorScroll(element, callback) {
+  const $element = $(element);
+
+  // return if element doesn't exist


These comments are stating what's pretty obvious in the code - in this case, it's not even 100% correct, because the callback is still executed.

Please explain why this is necessary, not what it does.

This reminds me of Clean Code, chapter 4 😄 🚀

@datitisev It seems like you also misunderstood me. I would love to see comments explaining why this is needed (because of something that happens before this code is called), not what error it prevents in that same method. That's mostly obvious from looking at the code.

What I don't see when looking at this code:

Why would the discussionList ever not exist when the post composer is open?

When would somebody pass a not-existing element into anchorScroll?

But, again, let's focus on the other PRs first.

Why would the discussionList ever not exist when the post composer is open?

I can only assume during redraws or something

When would somebody pass a not-existing element into anchorScroll?

Not intentionally. If you look at the stack trace, it was because of the PostStream. From there, I assume it tried to scroll to a post (through its index/number) that hadn't been loaded yet.

There's really not much I can gather from the stack traces, and this is a pretty simple fix. This would be a temporary solution, until (or if) we find and & solve the underlying issues.

But yeah, focus on other PRs first.

The post composer could have been closed in between scheduling and executing the callback. Fixes flarum/org#58. Refs #1881.

franzliedke · 2020-02-07T11:21:28Z

I fixed one of them (the ReplyPlaceholder one) in master as we've discovered the root cause here is a race condition (setInterval yadda-yadda).

Still unsure about the other ones. I prefer fixing root causes. 😉

The post composer could have been closed in between scheduling and executing the callback. Fixes flarum/org#58. Refs flarum#1881.

franzliedke · 2020-04-10T19:36:36Z

Please rebase. 🙏

franzliedke · 2020-04-17T10:31:55Z

Because we now auto-format our JS code with Prettier, this branch now has conflicts with master. Sorry about that. 😉

Please take the steps outlined in the forum to resolve the conflicts.

Sentry > DEMOS-8 TypeError: Cannot read property 'top' of undefined at apply(webpack://@flarum/core/./src/common/components/Dropdown.js:69:15) at apply(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:5237:1) at apply(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:5044:1) at trigger(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:8471:1) at call(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:8549:1) at each(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:367:1) at each(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:202:1) at trigger(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:8548:1) at apply(webpack://@flarum/core/./node_modules/bootstrap/js/dropdown.js:88:1) at apply(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:5237:1) at HTMLDocument.g.handle(webpack://@flarum/core/./node_modules/jquery/dist/jquery.js:5044:1)

Sentry > DEMOS-C TypeError: undefined is not an object (evaluating '.offset().top') at anchorScroll(webpack://@flarum/core/./src/common/utils/anchorScroll.js:16:37) at onSuccess(webpack://@flarum/core/./src/forum/components/PostStream.js:115:17) at notThennable(webpack://@flarum/core/./node_modules/mithril/mithril.js:2033:1) at thennable(webpack://@flarum/core/./node_modules/mithril/mithril.js:2004:1) at fire(webpack://@flarum/core/./node_modules/mithril/mithril.js:2024:1) at resolve(webpack://@flarum/core/./node_modules/mithril/mithril.js:1941:1) at then(webpack://@flarum/core/./node_modules/mithril/mithril.js:1962:1) at loadNearNumber(webpack://@flarum/core/./node_modules/mithril/mithril.js:1905:1) at document(webpack://@flarum/core/./src/forum/components/PostStream.js:66:26) at ? (webpack://@flarum/core/./src/forum/components/DiscussionPage.js:225:7)

Sentry > DEMOS-Q TypeError: Cannot read property 'addDiscussion' of undefined at onSuccess(webpack://@flarum/core/./src/forum/components/DiscussionComposer.js:95:34) at notThennable(webpack://@flarum/core/./node_modules/mithril/mithril.js:2033:1) at thennable(webpack://@flarum/core/./node_modules/mithril/mithril.js:2004:1) at fire(webpack://@flarum/core/./node_modules/mithril/mithril.js:2024:1) at resolve(webpack://@flarum/core/./node_modules/mithril/mithril.js:1941:1) at ? (webpack://@flarum/core/./node_modules/mithril/mithril.js:1976:1)

askvortsov1 · 2020-05-14T00:54:14Z

js/src/forum/components/DiscussionComposer.js

@@ -98,7 +98,9 @@ export default class DiscussionComposer extends ComposerBody {
      .save(data)
      .then((discussion) => {
        app.composer.hide();
-        app.cache.discussionList.refresh();
+
+        if (app.cache.discussionList) app.cache.discussionList.refresh(discussion);


This line should no longer be necessary after #2150

stale · 2020-08-12T03:05:28Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

franzliedke · 2020-08-14T09:07:40Z

I hope this will no longer be necessary with Typescript. Or if so, it will be hopelessly conflicted.

franzliedke suggested changes Sep 9, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

stale bot added the stale Issues that have had over 90 days of inactivity label Jan 18, 2020

franzliedke added a commit that referenced this pull request Feb 7, 2020

Fix race condition in post preview

2f3d999

The post composer could have been closed in between scheduling and executing the callback. Fixes flarum/org#58. Refs #1881.

stale bot removed the stale Issues that have had over 90 days of inactivity label Feb 7, 2020

wzdiyb pushed a commit to wzdiyb/core that referenced this pull request Feb 16, 2020

Fix race condition in post preview

353b52c

The post composer could have been closed in between scheduling and executing the callback. Fixes flarum/org#58. Refs flarum#1881.

dsevillamartin added 4 commits April 17, 2020 17:00

Explain why check is needed in comments

917a55b

dsevillamartin force-pushed the ds/fix-frontend-sentry-errors branch from 08a1db6 to 917a55b Compare April 17, 2020 21:02

askvortsov1 approved these changes May 13, 2020

View reviewed changes

tankerkiller125 approved these changes May 13, 2020

View reviewed changes

askvortsov1 reviewed May 14, 2020

View reviewed changes

stale bot added the stale Issues that have had over 90 days of inactivity label Aug 12, 2020

franzliedke closed this Aug 14, 2020

dsevillamartin deleted the ds/fix-frontend-sentry-errors branch August 14, 2020 12:51

snyk-bot mentioned this pull request Feb 25, 2022

[Snyk] Security upgrade mithril from 0.2.8 to 2.0.3 Matthelonianxl/core#29

Open

dsevillamartin mentioned this pull request Apr 18, 2022

fix: undefined element to anchorScroll triggers errors #3402

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix JS errors that only require a check for truthy value #1881

Fix JS errors that only require a check for truthy value #1881

dsevillamartin commented Sep 9, 2019

franzliedke left a comment

franzliedke Sep 9, 2019

dsevillamartin Sep 9, 2019

franzliedke Sep 9, 2019

matteocontrini Sep 9, 2019

franzliedke Sep 9, 2019

franzliedke Sep 9, 2019

dsevillamartin Sep 9, 2019 •

edited

This comment has been minimized.

franzliedke commented Feb 7, 2020

franzliedke commented Apr 10, 2020

franzliedke commented Apr 17, 2020

askvortsov1 May 14, 2020

stale bot commented Aug 12, 2020

franzliedke commented Aug 14, 2020

Fix JS errors that only require a check for truthy value #1881

Fix JS errors that only require a check for truthy value #1881

Conversation

dsevillamartin commented Sep 9, 2019

franzliedke left a comment

Choose a reason for hiding this comment

franzliedke Sep 9, 2019

Choose a reason for hiding this comment

dsevillamartin Sep 9, 2019

Choose a reason for hiding this comment

franzliedke Sep 9, 2019

Choose a reason for hiding this comment

matteocontrini Sep 9, 2019

Choose a reason for hiding this comment

franzliedke Sep 9, 2019

Choose a reason for hiding this comment

franzliedke Sep 9, 2019

Choose a reason for hiding this comment

dsevillamartin Sep 9, 2019 • edited

Choose a reason for hiding this comment

This comment has been minimized.

franzliedke commented Feb 7, 2020

franzliedke commented Apr 10, 2020

franzliedke commented Apr 17, 2020

askvortsov1 May 14, 2020

Choose a reason for hiding this comment

stale bot commented Aug 12, 2020

franzliedke commented Aug 14, 2020

dsevillamartin Sep 9, 2019 •

edited