Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HCRC-109 | fix(askem): fix Askem looping to fetch rns.js on article pages #529

Merged
merged 3 commits into from
Nov 6, 2023
Merged

HCRC-109 | fix(askem): fix Askem looping to fetch rns.js on article pages #529

merged 3 commits into from
Nov 6, 2023

Conversation

karisal-anders
Copy link
Contributor

@karisal-anders karisal-anders commented Nov 1, 2023 •
edited by jira bot
Loading

Description

fix(askem): fix Askem looping to fetch rns.js on article pages

NOTE:

  • There seems to be an underlying issue here that is not fully fixed
    by this commit. This commit fixes the infinite looping to fetch
    Askem's rns.js script and the unnecessary updating of AskemInstance
    but it's a tip of the iceberg so to speak, there's still something
    being rerendered underneath. So, more work is needed to fix the
    underlying issue.

parts:

  • don't render AskemProvider and AskemFeedbackContainer on server-side
  • don't update used AskemInstance except when it really changes, use
    deep equality to compare

also:

  • add more fields and documentation to Askem's data using
    https://docs.reactandshare.com/
  • pass all the Askem fields to window.rnsData
  • fix AskemContext and AskemProvider default import renaming typos

refs HCRC-106

refactor: improve Askem api key api config

refactor: move Matomo, Askem and HDS-style handling of the BaseApp to new files

HCRC-106

Issues

Closes

HCRC-109

Related

Testing

  • checkout PR's branch locally because review environments don't have Askem enabled
  • cd apps/sports-helsinki
  • yarn
  • yarn dev
  • open page https://localhost:3000/fi/articles/yleinen/testiartikkeli-syyskuu in browser
  • make sure Askem cookies are allowed, if you're not sure clear localhost:3000's cookies or at least the city-of-helsinki-cookie-consents cookie
  • do twice in a row:
    • scroll down to the bottom of the page
    • scroll back up to the top of the page
    • reload

What is expected:

  • Everything should be fine and the network shouldn't be loading anything in a seemingly infinite loop.
  • Askem functionality should also work all around correctly.

Automated tests

Manual testing

Tested locally and couldn't get the infinite network loop fetching Askem's rns.js script to happen anymore.

Screenshots

Additional notes

@terovirtanen
Copy link
Contributor

Events-Helsinki branch is deployed to platta: https://tapahtumat-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

Hobbies-Helsinki branch is deployed to platta: https://harrastukset-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://tapahtumat-pr529.dev.hel.ninja 😆🎉🎉🎉

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://harrastukset-pr529.dev.hel.ninja 😆🎉🎉🎉

@terovirtanen
Copy link
Contributor

Sports-Helsinki branch is deployed to platta: https://liikunta-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

VENUE GRAPHQL PROXY is deployed to platta: https://venue-graphql-proxy-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

EVENTS GRAPHQL PROXY is deployed to platta: https://events-graphql-proxy-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://liikunta-pr529.dev.hel.ninja 😆🎉🎉🎉

nikomakela
nikomakela reviewed Nov 3, 2023
View reviewed changes
packages/components/src/app/BaseApp.tsx Outdated
Comment on lines 142 to 150
const newAskemConfiguration: AskemConfigs = {
...askemConfigurationInput,
consentGiven: askemConsentGiven,
};

if (!askemInstance || !isEqual(askemConfiguration, newAskemConfiguration)) {
setAskemConfiguration(newAskemConfiguration);
setAskemInstance(createAskemInstance(newAskemConfiguration));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this. Handling this wit a state should not make a difference compared to a memo. The memo is just a lot cleaner. Both are executed when the dependency array changes.

I need to test this. How was the issue reproducable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found the instructions from the PR description 😝

Testing
checkout PR's branch locally because review environments don't have Askem enabled
cd apps/sports-helsinki
yarn
yarn dev
open page https://localhost:3000/fi/articles/yleinen/testiartikkeli-syyskuu in browser
make sure Askem cookies are allowed, if you're not sure clear localhost:3000's cookies or at least the city-of-helsinki-cookie-consents cookie
do twice in a row:
scroll down to the bottom of the page
scroll back up to the top of the page
reload
What is expected:

Everything should be fine and the network shouldn't be loading anything in a seemingly infinite loop.
Askem functionality should also work all around correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useMemo with any object variables is relatively useless IMO as it doesn't compare the objects' contents but simply the references themselves → useMemo always changes the return value if any of useMemo's dependency list's objects' references change even though their contents stay the same. I added deep equality checking to get around this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually can compare deeply: the dependency array could be something like [JSON.stringify(object)]. As a reference I found this for example: https://javascript.plainenglish.io/a-less-known-trick-with-react-hook-dependencies-8dafaca7a150

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to useMemo documentation it seems they're suggesting to memoize objects used as useMemo dependencies → recursively going back to only using primitive types as dependencies in useMemo in the leaf nodes of the useMemo calls.

The JSON.stringify has some limitations:

  • BigInt -> throw
  • undefined -> null
  • Function -> nothing or null
  • Symbol -> nothing or null
  • NaN -> null
  • Infinity -> null
  • Map -> "{}"
  • Set -> "{}"

which could possibly be overcome with the use of the replacer parameter.

@nikomakela nikomakela self-requested a review November 3, 2023 12:59
nikomakela
nikomakela requested changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@nikomakela nikomakela left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test results

❌ I could still reproduce the issue locally ❗

image

image

This actually happens even when the askem is disabled, so maybe it's not related to Askem at all 🤔 :

image

Whole footer is loaded infinitely:

image

@nikomakela
Copy link
Contributor

Okay, I'm 99% sure, that the problem is in the LayoutCards-module (that is also in the example page of this PR)

When I exclude all the LayoutCard modules, the example page renders OK


const NextCmsArticle: NextPage<{
  article: ArticleType;
  breadcrumbs: Breadcrumb[] | null;
  collections: CollectionType[];
}> = ({ article, breadcrumbs, collections }) => {
  const {
    utils: { getRoutedInternalHref },
  } = useConfig();

  const { t: commonTranslation } = useCommonTranslation();
  const { t: appTranslation } = useAppSportsTranslation();
  const { footerMenu } = useContext(NavigationContext);
  console.log('ARTICLE', { article });

  // FIXME: Return null to fix SSR rendering for notFound-page.
  // This is needed only with fallback: true, but should not be needed at all.
  if (!article) return null;

  return (
    <MatomoWrapper>
      <RHHCPage
        className="article-page"
        navigation={<Navigation page={article} />}
        content={
          <>
            <RouteMeta origin={AppConfig.origin} page={article} />
            <RHHCPageContent
              page={
                {
                  ...article,
                  modules: [
                    ...article.modules?.filter(
                      (a) => a?.__typename !== 'LayoutCards'
                    ),
                  ],
                } as ArticleType
              }
              heroContainer={<KorosWrapper />}
              breadcrumbs={
                breadcrumbs && breadcrumbs.length > 0 ? breadcrumbs : undefined
              }
              shareLinks={
                <ShareLinks title={commonTranslation('common:share.article')} />
              }
              collections={
                collections
                  ? cmsHelper.getDefaultCollections(
                      article,
                      getRoutedInternalHref
                    )
                  : []
              }
            />
          </>
        }
        footer={
          <FooterSection
            menu={footerMenu}
            appName={appTranslation('appSports:appName')}
          />
        }
      />
    </MatomoWrapper>
  );
};

@nikomakela nikomakela mentioned this pull request Nov 3, 2023
Merged
@nikomakela
Copy link
Contributor

nikomakela commented Nov 3, 2023 •
edited
Loading

There are 2 commits that I call improvements in #532 ! 😌

I think that the changes in this PR could be taken in action, but it just does not fix what it is saying to fix.

@terovirtanen
Copy link
Contributor

Hobbies-Helsinki branch is deployed to platta: https://harrastukset-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

Events-Helsinki branch is deployed to platta: https://tapahtumat-pr529.dev.hel.ninja 🚀🚀🚀

@karisal-anders
fix(askem): fix Askem looping to fetch rns.js on article pages
e8b7109 
NOTE:
 - There seems to be an underlying issue here that is not fully fixed
   by this commit. This commit fixes the infinite looping to fetch
   Askem's rns.js script and the unnecessary updating of AskemInstance
   but  it's a tip of the iceberg so to speak, there's still something
   being rerendered underneath. So, more work is needed to fix the
   underlying issue.

parts:
 - don't render AskemProvider and AskemFeedbackContainer on server-side
 - don't update used AskemInstance except when it really changes, use
   deep equality to compare

also:
 - add more fields and documentation to Askem's data using
   https://docs.reactandshare.com/
 - pass all the Askem fields to window.rnsData
 - fix AskemContext and AskemProvider default import renaming typos

refs HCRC-106
@terovirtanen
Copy link
Contributor

TestCafe result is success for https://harrastukset-pr529.dev.hel.ninja 😆🎉🎉🎉

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://tapahtumat-pr529.dev.hel.ninja 😆🎉🎉🎉

@terovirtanen
Copy link
Contributor

Sports-Helsinki branch is deployed to platta: https://liikunta-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

VENUE GRAPHQL PROXY is deployed to platta: https://venue-graphql-proxy-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://liikunta-pr529.dev.hel.ninja 😆🎉🎉🎉

@karisal-anders karisal-anders changed the title HCRC-106 | fix(askem): fix Askem component infinitely reloading on article pages HCRC-106 | fix(askem): fix Askem looping to fetch rns.js on article pages Nov 6, 2023
@terovirtanen
Copy link
Contributor

EVENTS GRAPHQL PROXY is deployed to platta: https://events-graphql-proxy-pr529.dev.hel.ninja 🚀🚀🚀

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@terovirtanen
Copy link
Contributor

Events-Helsinki branch is deployed to platta: https://tapahtumat-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

Hobbies-Helsinki branch is deployed to platta: https://harrastukset-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://tapahtumat-pr529.dev.hel.ninja 😆🎉🎉🎉

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://harrastukset-pr529.dev.hel.ninja 😆🎉🎉🎉

@terovirtanen
Copy link
Contributor

Sports-Helsinki branch is deployed to platta: https://liikunta-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

VENUE GRAPHQL PROXY is deployed to platta: https://venue-graphql-proxy-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

EVENTS GRAPHQL PROXY is deployed to platta: https://events-graphql-proxy-pr529.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://liikunta-pr529.dev.hel.ninja 😆🎉🎉🎉

@karisal-anders karisal-anders changed the title HCRC-106 | fix(askem): fix Askem looping to fetch rns.js on article pages HCRC-109 | fix(askem): fix Askem looping to fetch rns.js on article pages Nov 6, 2023
@karisal-anders
Copy link
Contributor Author

There are 2 commits that I call improvements in #532 ! 😌

I think that the changes in this PR could be taken in action, but it just does not fix what it is saying to fix.

The commits from #532 are merged into this PR.

Changed the PR's name and the first commit's commit title to reflect what is being done:

  • fix(askem): fix Askem looping to fetch rns.js on article pages

Also added a note to the first commit's commit message:

NOTE:
 - There seems to be an underlying issue here that is not fully fixed
   by this commit. This commit fixes the infinite looping to fetch
   Askem's rns.js script and the unnecessary updating of AskemInstance
   but  it's a tip of the iceberg so to speak, there's still something
   being rerendered underneath. So, more work is needed to fix the
   underlying issue.

@karisal-anders karisal-anders merged commit d55cf3d into main Nov 6, 2023
104 checks passed
@karisal-anders karisal-anders deleted the HCRC-106-fix-askem branch November 6, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomakela nikomakela nikomakela approved these changes

@melniiv melniiv Awaiting requested review from melniiv

@jakezu jakezu Awaiting requested review from jakezu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@karisal-anders @terovirtanen @nikomakela