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.

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

feat: make optimizely script asynchronous #1007

Closed
wants to merge 34 commits into from

Conversation

brobro10000
Copy link
Contributor

Adds async attribute to each optimizely script to allow asynchronous loading with the loaders

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)
  • Ensure English strings are marked for translation. See documentation for more details.

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

adamstankiewicz and others added 30 commits February 22, 2024 14:58
…al-enterprise into feat/react-query-route-loaders
* feat: refactors away customer-agreement endpoint

* feat: refactors away customer-agreement endpoint

* chore: PR feedback

* fix: abstracts response transform and defines distinct subscriptions object

* chore: refactor to match new license manager refactor

* chore: tests

* chore: PR feedback

* chore: PR feedback
* feat: migrate existing API calls to React Query and Route Loaders

* feat: Catch errors around root loader (#962)

* fix: remove hardcoded hex colors where applicable, DRY using css vars in JS (#964)

* feat: resolve all data during route transitions (#965)

* fix: handle when active enterprise is changed external to current page view (#966)

* fix: use * for not found page (#967)

* feat: adds global queryKey object for useQuery

* chore: Merge cleanup

* chore: PR feedback and refactor additional endpoints

* chore: PR fixes 2

* feat: query-key-factory

* chore: PR feedback

* chore: refactor to new queries

* chore: merge fix

* chore: PR feedback

* chore: pr cleanup

* chore: Rename functions

* chore: jsdoc added

* chore: jsdoc added

* chore: Merge fixes

* chore: PR feedback

* chore: merge fixes

* chore: PR feedback

* chore: disable _ctx lint error

---------

Co-authored-by: Adam Stankiewicz <agstanki@gmail.com>
* feat: refactor notices provider to useQuery

* chore: merge fix

* chore: cleanup

* fix: redirects without momentary render

* fix: debugging

* chore: Testing and cleanup

* chore: PR feedback
* feat: integrate search page with useQuery

* chore: refactor

* feat: SearchCatalog abstrated from UserSubsidyContext

* feat: render search page

* chore: cleanup

* chore: cleanup 2

* fix: dependency error

* feat: migrate content highlights

* feat: migrate academies to loader and queryKeyFactory

* chore: lint

* fix: update quey key hierarchy for academies

* chore: linting

* fix: useAcademies return

* chore: PR feedback
* feat: Follow up to search page integration

* fix: more memoization to prevent rerenders

* fix: update card memoization

* chore: Search component test updates

* chore: content highlights tests under search directory updated

* chore: Search cards and results tests updated

* chore: hooks for search tests updated

* feat: pathway modal route

* chore: pathway card tests

* chore: PR feedback and test optimization

* chore: Search academy updated

* chore: PR feedback
Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

A couple sanity check type questions, and a nit that the indentation in the file could use some clean up now. For example, the opening/closing template tags are not aligned and <head> is no longer nested with indents under <html>.

<% if (htmlWebpackPlugin.options.NODE_ENV==='production' && htmlWebpackPlugin.options.OPTIMIZELY_PROJECT_ID &&
htmlWebpackPlugin.options.DEPLOYMENT_ENV==='production' ) { %>
<script src="https://www.edx.org/optimizelyjs/<%= htmlWebpackPlugin.options.OPTIMIZELY_PROJECT_ID %>.js"
async="async"></script>
Copy link
Member

Choose a reason for hiding this comment

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

[sanity check] Would we want async or defer?

If the async attribute is set, the script is downloaded in parallel to parsing the page, and executed as soon as it is available. The parsing of the page is interrupted once the script is downloaded completely, and then the script is executed, before the parsing of the rest of the page continues.

[sanity check x2] Is there any risk that we'd be trying to execute Optimizely related code paths before it's fully loaded after these changes? E.g., right now, I believe the earliest call to Optimizely is within EnterprisePage where pushUserCustomerAttributes is called. However, what if this call was actually done within a route loader? With the Optimizely script downloaded async/deferred, can we still guarantee window.optimizely will exist when we need it to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sanity check 1: I considered both but the safer option of the two was async imo. By allowing the execution of the script as soon as its available should reduce the chances of the resources being unavailable on page load. defer is an option as well and might be the more performant option but we run the risk of the resources that not being available...

Sanity check 2: ...that being said, I ran a couple of instances on localhost stage with the request blocked and the page did not error out. By parsing the code they all seem to be gated by a check for optimizely before they execute within this utility file. Given the appropriate checks we COULD use defer as opposed to async.

Copy link
Member

Choose a reason for hiding this comment

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

[update, follow-up from Slack] We decided to defer on this approach for now given the uncertainty around timing when making Optimizely a non-render-blocking resource (i.e., how do we delay execution of Optimizely helper functions until we know for sure window.optimizely has finished loading?). This uncertainty could lead to unintended loss of data pushed up to Optimizely (user identify calls, events, etc.).

Helpful resource: https://support.optimizely.com/hc/en-us/articles/4410289790221-Install-the-snippet-as-a-non-blocking-resource

Base automatically changed from feat/react-query-route-loaders to master April 4, 2024 16:54
@brobro10000
Copy link
Contributor Author

Closing pull request mainly due to the way optimizely is designed. It is intentionally meant to be a non deferred request.

@brobro10000 brobro10000 deleted the hu/ent-optimizely-performance branch May 13, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants