Skip to content
This repository has been archived by the owner on Jun 13, 2024. It is now read-only.

Build experimental-bundled-ui-shell.js as immediately invoked function expression #227

Merged
merged 2 commits into from
May 3, 2023

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Apr 20, 2023

Changes

As found in Qiskit/qiskit_sphinx_theme#274 (comment), we want to bundle web components into qiskit_sphinx_theme as immediately invoked function expressions. That allows us to avoid issues with CORS on local development & results in the component being immediately loaded, rather than having a delay.

Implementation details

See https://rollupjs.org/configuration-options/#output-format for the relevant docs.

According to Abdón, experimental-bundled-ui-shell.js is only used by qiskit_sphinx_theme, so this should have no impact on other teams. It will not change e.g. how npm install works, as far as I can tell.

@changeset-bot
Copy link

changeset-bot bot commented Apr 20, 2023

⚠️ No Changeset found

Latest commit: a80d402

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Eric-Arellano added a commit to Qiskit/qiskit_sphinx_theme that referenced this pull request Apr 25, 2023
# Motivation

We decided to stop loading web components via the Internet (e.g. a CDN
like unpkg.com) because:

1. It introduced a new point of failure. If the service goes down, our
site is degraded. See
#246
2. There is a delay in loading the web component because we first render
the HTML before finishing pulling the web component

Related, we also decided to switch from always using the latest version
of web components (by always pointing to the same URL) to instead
versioning the component. For example, the Sphinx Theme 1.14 might use a
different version of the same component than Theme 1.11. We believe it
is not very painful to update web components thanks to now having
release branches & automated releases. It is much more stable to pin the
version of the web component, whereas before, updating the URL impacted
every single Theme release without an opportunity to validate it makes
sense for each release.

# FYI: How we load the file

We use an "immediately invoked function expression" (IIFE). That makes
sure that the top nav bar is rendered at the same time as the general
site, unlike before. See
Qiskit/web-components#227 for the change to have
the web-components repo build the file this way.

## Rejected: ES6 module via a local file

We'd still use `type="module"` like we were using before, but change the
`href`/`src` to point to a local file path rather than URL.

This breaks opening up `index.html`. We get a CORS issue that the file
cannot be accessed. That happens because `<script type="module">` has
stricter CORS policies. So, you now need to use a server like running
`python -m http.server` inside the `_build/html` folder for the site to
render properly.

This also still has a delay in the top nav bar rendering. That is
because ES6 modules have deferred execution, meaning that the JavaScript
runs after the HTML page is set up.

## Rejected: inlining the JavaScript module

We can avoid the CORS issue from "ES6 module via a local file" by
inlining the JavaScript with a Jinja `include` directive.

But, that increases the file size of each HTML page by 184KB. It means
that we keep downloading the same duplicate JS code because the browser
cannot cache the dedicated web component file.

This approach also still results in a delay in loading the top nav bar
because ES6 modules have deferred execution.
@Eric-Arellano
Copy link
Contributor Author

Bump @y4izus @eddybrando

Copy link
Contributor

@y4izus y4izus left a comment

Choose a reason for hiding this comment

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

Thank you Eric!!

@y4izus y4izus merged commit aa9ac5a into Qiskit:main May 3, 2023
@Eric-Arellano Eric-Arellano deleted the ui-shell-component-iife branch May 3, 2023 15:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants