Skip to content

Conversation

gre
Copy link
Contributor

@gre gre commented Aug 25, 2022

📝 Description

In production, window.analytics can randomly be undefined at load time. This is very likely due to the order of the scripts.

This PR do a bunch of changes:

  • it put scripts back in order, basically we must set the <script> that init the analytics object in window first.
  • it fixes the HTML!, the usage of <script src=".." /> is bad, it should be <script src=".."></script> because otherwise, the next HTML that gets after the first code would be "EATEN"! Screenshot 2022-08-25 at 14 05 18 (NB: there is a possibility that it was the actual bug at stake, but for some reason i don't reproduce myself)
  • it uses defer which seems more relevant than "async" but it's a subtility.
  • if the issue ever happen again, we would get error sent to Sentry (logger.critical does just that) to be sure we are informed of the problem if it occurs again.
  • The test will also verify userId is settled in the first event.

❓ Context

  • Impacted projects: LLD
  • Linked resource(s): live-3420

✅ Checklist

  • Test coverage
  • Atomic delivery
  • No breaking changes

📸 Demo

🚀 Expectations to reach

Please make sure you follow these Important Steps.

Pull Requests must pass the CI and be internally validated in order to be merged.

@gre gre requested a review from Justkant August 25, 2022 12:21
@vercel
Copy link

vercel bot commented Aug 25, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
live-common-tools ✅ Ready (Inspect) Visit Preview Aug 25, 2022 at 0:23AM (UTC)
3 Ignored Deployments
Name Status Preview Updated
ledger-live-github-bot ⬜️ Ignored (Inspect) Aug 25, 2022 at 0:23AM (UTC)
native-ui-storybook ⬜️ Ignored (Inspect) Aug 25, 2022 at 0:23AM (UTC)
react-ui-storybook ⬜️ Ignored (Inspect) Aug 25, 2022 at 0:23AM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Aug 25, 2022

🦋 Changeset detected

Latest commit: d2fb65b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
ledger-live-desktop Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

@gre

Screenshots: ✅

There are no changes in the screenshots for this PR. If this is expected, you are good to go.

@gre gre merged commit 4fe5968 into develop Aug 29, 2022
@gre gre deleted the bugfix/live-3420 branch August 29, 2022 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants