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

fix: use @lit/context #2131

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

fix: use @lit/context #2131

wants to merge 12 commits into from

Conversation

bennypowers
Copy link
Member

@bennypowers bennypowers commented Jan 16, 2025

This PR is based on and requires patternfly/patternfly-elements#2891

What I did

  1. replace our custom context protocol implementation with lit's

Testing Instructions

  1. disable javascript
  2. visit card darkest demo
  3. cta should render with correct colours
  4. reenable js
  5. same should be true
  6. inspect the rest of the color and heading context demos (tbd)

Notes to Reviewers

pay special attention to the heading level elements, this may require a wholesale refactor to get working under the new context regime.

Copy link

changeset-bot bot commented Jan 16, 2025

⚠️ No Changeset found

Latest commit: 9f048b1

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

Copy link

netlify bot commented Jan 16, 2025

Deploy Preview for red-hat-design-system ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/red-hat-design-system/deploys/678ced6e32c69c0719bf280b
😎 Deploy Preview https://deploy-preview-2131--red-hat-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Jan 16, 2025

Size Change: -4.52 kB (-2.19%)

Total Size: 202 kB

Filename Size Change
./elements/rh-accordion/rh-accordion-header.js 2.77 kB +79 B (+2.93%)
./elements/rh-accordion/rh-accordion.js 3.24 kB +26 B (+0.81%)
./elements/rh-audio-player/rh-audio-player-about.js 1.81 kB +7 B (+0.39%)
./elements/rh-audio-player/rh-audio-player-subscribe.js 1.39 kB +9 B (+0.65%)
./elements/rh-audio-player/rh-cue.js 1.96 kB +5 B (+0.26%)
./elements/rh-audio-player/rh-transcript.js 2.68 kB -21 B (-0.78%)
./elements/rh-card/rh-card.js 3.58 kB +9 B (+0.25%)
./elements/rh-code-block/rh-code-block.js 7.37 kB +22 B (+0.3%)
./elements/rh-cta/rh-cta.js 4.04 kB +9 B (+0.22%)
./elements/rh-table/rh-table.js 3.49 kB +3 B (+0.09%)
./elements/rh-tabs/rh-tab.js 3.05 kB +34 B (+1.13%)
./lib/context/color/consumer.js 497 B -910 B (-64.68%) 🏆
./lib/context/color/controller.js 0 B -947 B (removed) 🏆
./lib/context/color/provider.js 937 B -1.24 kB (-57%) 🏆
./lib/context/event.js 0 B -593 B (removed) 🏆
./lib/context/headings/consumer.js 317 B -405 B (-56.09%) 🏆
./lib/context/headings/controller.js 0 B -1.12 kB (removed) 🏆
./lib/context/headings/provider.js 1.02 kB -217 B (-17.57%) 👏
./uxdot/uxdot-sidenav.js 2.67 kB -2 B (-0.07%)
./lib/context/color/context.js 199 B +199 B (new file) 🆕
./lib/context/headings/context.js 532 B +532 B (new file) 🆕
ℹ️ View Unchanged
Filename Size
./elements.js 473 B
./elements/rh-accordion/context.js 162 B
./elements/rh-accordion/rh-accordion-panel.js 1.35 kB
./elements/rh-alert/rh-alert.js 4.26 kB
./elements/rh-audio-player/rh-audio-player-rate-stepper.js 1.85 kB
./elements/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 1.53 kB
./elements/rh-audio-player/rh-audio-player.js 13.1 kB
./elements/rh-avatar/random-pattern-controller.js 2.72 kB
./elements/rh-avatar/rh-avatar.js 2.85 kB
./elements/rh-back-to-top/rh-back-to-top.js 2.1 kB
./elements/rh-badge/rh-badge.js 1.55 kB
./elements/rh-blockquote/rh-blockquote.js 1.36 kB
./elements/rh-breadcrumb/rh-breadcrumb.js 1.5 kB
./elements/rh-button/rh-button.js 4.24 kB
./elements/rh-code-block/prism.css.js 667 B
./elements/rh-code-block/prism.js 572 B
./elements/rh-dialog/rh-dialog.js 4.78 kB
./elements/rh-dialog/yt-api.js 617 B
./elements/rh-footer/rh-footer-block.js 714 B
./elements/rh-footer/rh-footer-copyright.js 362 B
./elements/rh-footer/rh-footer-links.js 1.17 kB
./elements/rh-footer/rh-footer-social-link.js 964 B
./elements/rh-footer/rh-footer-universal.js 3.99 kB
./elements/rh-footer/rh-footer.js 4.96 kB
./elements/rh-health-index/rh-health-index.js 2.35 kB
./elements/rh-icon/rh-icon.js 2.47 kB
./elements/rh-icon/ssr.js 181 B
./elements/rh-menu/rh-menu.js 1.29 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 2.47 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 1.3 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu.js 1.75 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-overlay.js 571 B
./elements/rh-navigation-secondary/rh-navigation-secondary.js 5.26 kB
./elements/rh-navigation-secondary/test/fixtures.js 769 B
./elements/rh-pagination/rh-pagination.js 5.4 kB
./elements/rh-site-status/rh-site-status.js 2.5 kB
./elements/rh-skip-link/rh-skip-link.js 1.19 kB
./elements/rh-spinner/rh-spinner.js 1.38 kB
./elements/rh-stat/rh-stat.js 2.13 kB
./elements/rh-subnav/rh-subnav.js 2.73 kB
./elements/rh-surface/rh-surface.js 1.14 kB
./elements/rh-surface/test/elements.js 423 B
./elements/rh-switch/rh-switch.js 2.93 kB
./elements/rh-table/rh-sort-button.js 1.49 kB
./elements/rh-tabs/context.js 160 B
./elements/rh-tabs/rh-tab-panel.js 1.04 kB
./elements/rh-tabs/rh-tabs.js 3.77 kB
./elements/rh-tag/rh-tag.js 2.79 kB
./elements/rh-tile/rh-tile-group.js 1.81 kB
./elements/rh-tile/rh-tile.js 5.09 kB
./elements/rh-timestamp/rh-timestamp.js 983 B
./elements/rh-tooltip/rh-tooltip.js 2.73 kB
./elements/rh-video-embed/rh-video-embed.js 4.59 kB
./lib/DirController.js 565 B
./lib/elements/rh-context-demo/rh-context-demo.js 1.28 kB
./lib/elements/rh-context-picker/rh-context-picker.js 2.24 kB
./lib/environment.js 194 B
./lib/functions.js 175 B
./lib/I18nController.js 1.38 kB
./lib/ScreenSizeController.js 849 B
./lib/ssr-controller.js 251 B
./react/rh-accordion/rh-accordion-header.js 199 B
./react/rh-accordion/rh-accordion-panel.js 185 B
./react/rh-accordion/rh-accordion.js 215 B
./react/rh-alert/rh-alert.js 184 B
./react/rh-audio-player/rh-audio-player-about.js 191 B
./react/rh-audio-player/rh-audio-player-rate-stepper.js 213 B
./react/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 214 B
./react/rh-audio-player/rh-audio-player-subscribe.js 196 B
./react/rh-audio-player/rh-audio-player.js 183 B
./react/rh-audio-player/rh-cue.js 195 B
./react/rh-audio-player/rh-transcript.js 207 B
./react/rh-avatar/rh-avatar.js 173 B
./react/rh-back-to-top/rh-back-to-top.js 183 B
./react/rh-badge/rh-badge.js 174 B
./react/rh-blockquote/rh-blockquote.js 179 B
./react/rh-breadcrumb/rh-breadcrumb.js 179 B
./react/rh-button/rh-button.js 174 B
./react/rh-card/rh-card.js 172 B
./react/rh-code-block/rh-code-block.js 181 B
./react/rh-cta/rh-cta.js 170 B
./react/rh-dialog/rh-dialog.js 203 B
./react/rh-footer/rh-footer-block.js 184 B
./react/rh-footer/rh-footer-copyright.js 187 B
./react/rh-footer/rh-footer-links.js 185 B
./react/rh-footer/rh-footer-social-link.js 193 B
./react/rh-footer/rh-footer-universal.js 188 B
./react/rh-footer/rh-footer.js 174 B
./react/rh-health-index/rh-health-index.js 184 B
./react/rh-icon/rh-icon.js 202 B
./react/rh-menu/rh-menu.js 173 B
./react/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 217 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 205 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu.js 199 B
./react/rh-navigation-secondary/rh-navigation-secondary-overlay.js 201 B
./react/rh-navigation-secondary/rh-navigation-secondary.js 213 B
./react/rh-pagination/rh-pagination.js 178 B
./react/rh-site-status/rh-site-status.js 181 B
./react/rh-skip-link/rh-skip-link.js 181 B
./react/rh-spinner/rh-spinner.js 175 B
./react/rh-stat/rh-stat.js 171 B
./react/rh-subnav/rh-subnav.js 175 B
./react/rh-surface/rh-surface.js 175 B
./react/rh-switch/rh-switch.js 185 B
./react/rh-table/rh-sort-button.js 213 B
./react/rh-table/rh-table.js 174 B
./react/rh-tabs/rh-tab-panel.js 181 B
./react/rh-tabs/rh-tab.js 187 B
./react/rh-tabs/rh-tabs.js 174 B
./react/rh-tag/rh-tag.js 182 B
./react/rh-tile/rh-tile-group.js 183 B
./react/rh-tile/rh-tile.js 194 B
./react/rh-timestamp/rh-timestamp.js 176 B
./react/rh-tooltip/rh-tooltip.js 175 B
./react/rh-video-embed/rh-video-embed.js 227 B
./uxdot/uxdot-best-practice.js 742 B
./uxdot/uxdot-copy-button.js 1.2 kB
./uxdot/uxdot-copy-permalink.js 1.1 kB
./uxdot/uxdot-example.js 1.17 kB
./uxdot/uxdot-feedback.js 727 B
./uxdot/uxdot-header.js 1.07 kB
./uxdot/uxdot-hero.js 679 B
./uxdot/uxdot-installation-tabs.js 675 B
./uxdot/uxdot-masthead.js 809 B
./uxdot/uxdot-pattern-ssr-controller-client.js 604 B
./uxdot/uxdot-pattern-ssr-controller-server.js 1.68 kB
./uxdot/uxdot-pattern-ssr-controller.js 213 B
./uxdot/uxdot-pattern.js 2.12 kB
./uxdot/uxdot-repo-status-checklist.js 1.16 kB
./uxdot/uxdot-repo-status-list.js 1.07 kB
./uxdot/uxdot-repo-status-table.js 782 B
./uxdot/uxdot-repo.js 1.17 kB
./uxdot/uxdot-search.js 2.39 kB
./uxdot/uxdot-spacer-tokens-table.js 2.45 kB
./uxdot/uxdot-toc.js 1.13 kB

compressed-size-action

@bennypowers
Copy link
Member Author

at the moment, it looks like the callback for context consumers isn't being called. either i missed something simple or maybe there's an order of operations issue

@bennypowers
Copy link
Member Author

Ran some debug today. I believe we have a race condition in the order of imports. See the minimal repro below which successfully computes the context for a cta inside a card. the min repro has two major changes from prod:

  1. it does the ssr on the main thread instead of in a worker, and
  2. it only imports card and cta, nothing else
Working minimal Reproduction case
import type { LitElement, ReactiveController } from 'lit';
import type { RenderInfo, RenderResult } from '@lit-labs/ssr';
import type { RHDSSSRController } from '@rhds/elements/lib/ssr-controller.ts';

import '@patternfly/pfe-core/ssr-shims.js';
import '@lit-labs/ssr-dom-shim';

import { LitElementRenderer } from '@lit-labs/ssr/lib/lit-element-renderer.js';

import { writeFile } from 'node:fs/promises';
import { register } from 'node:module';
import { register as registerTS } from 'tsx/esm/api';

import { html } from 'lit';
import { render } from '@lit-labs/ssr';
import { collectResult } from '@lit-labs/ssr/lib/render-result.js';

const tsconfig = './tsconfig.settings.json';

register('./docs/_plugins/lit-ssr/lit-css-node.ts', import.meta.url);

registerTS({ tsconfig });

await import ('./elements/rh-card/rh-card.ts');
await import ('./elements/rh-cta/rh-cta.ts');

class RHDSSSRableRenderer extends LitElementRenderer {
  static isRHDSSSRController(ctrl: ReactiveController): ctrl is RHDSSSRController {
    return !!(ctrl as RHDSSSRController).isRHDSSSRController;
  }

  getControllers() {
    const element = (this.element as LitElement & { _$EO: Set<ReactiveController> });
    return Array.from(element._$EO ?? new Set())
        .filter(RHDSSSRableRenderer.isRHDSSSRController);
  }

  async setupController(controller: RHDSSSRController, renderInfo: RenderInfo) {
    if (controller.ssrSetup) {
      await controller.ssrSetup(renderInfo);
    }
    return '';
  }

  override* renderShadow(renderInfo: RenderInfo): RenderResult {
    for (const controller of this.getControllers()) {
      yield this.setupController(controller, renderInfo);
    }
    yield* super.renderShadow(renderInfo);
  }
}

class UnsafeHTMLStringsArray extends Array {
  public raw: readonly string[];
  constructor(string: string) {
    super();
    this.push(string);
    this.raw = [string];
  }
}

// Lit SSR includes comment markers to track the outer template from
// the template we've generated here, but it's not possible for this
// outer template to be hydrated, so they serve no purpose.
function trimOuterMarkers(renderedContent: string) {
  return renderedContent
      .replace(/^((<!--[^<>]*-->)|(<\?>)|\s)+/, '')
      .replace(/((<!--[^<>]*-->)|(<\?>)|\s)+$/, '');
}

const tpl = html(new UnsafeHTMLStringsArray(/* html */`
  <!DOCTYPE html>
  <html lang="en">
    <head>
      <meta charset="utf-8">
      <meta name="viewport" content="width=device-width, initial-scale=1">
      <title>context consumer noop repro</title>
      <script type="importmap">
        {
          "imports": {
            "@lit-labs/ssr-client/lit-element-hydrate-support.js": "/assets/packages/@lit-labs/ssr-client/lit-element-hydrate-support.js",
            "@lit/context": "/assets/packages/@lit/context/index.js",
            "@lit/reactive-element": "/assets/packages/@lit/reactive-element/reactive-element.js",
            "@lit/reactive-element/decorators/": "/assets/packages/@lit/reactive-element/decorators/",
            "@patternfly/elements": "/assets/packages/@patternfly/elements/pfe.min.js",
            "@patternfly/elements/": "/assets/packages/@patternfly/elements/",
            "@patternfly/icons/": "/assets/packages/@patternfly/icons/",
            "@patternfly/pfe-core": "https://ga.jspm.io/npm:@patternfly/[email protected]/core.js",
            "@patternfly/pfe-core/": "/assets/packages/@patternfly/pfe-core/",
            "@rhds/elements/": "/assets/packages/@rhds/elements/elements/",
            "@rhds/elements/lib/": "/assets/packages/@rhds/elements/lib/",
            "@rhds/icons/": "/assets/packages/@rhds/icons/",
            "@rhds/icons/icons.js": "/assets/packages/@rhds/icons/icons.js",
            "@rhds/tokens/": "/assets/packages/@rhds/tokens/js/",
            "@rhds/tokens/css/": "/assets/packages/@rhds/tokens/css/",
            "lit": "/assets/packages/lit/index.js",
            "lit-element": "/assets/packages/lit-element/index.js",
            "lit-element/lit-element.js": "/assets/packages/lit-element/lit-element.js",
            "lit-html": "/assets/packages/lit-html/lit-html.js",
            "lit-html/": "/assets/packages/lit-html/",
            "lit/": "/assets/packages/lit/",
            "lit/decorators/custom-element.js": "/assets/packages/lit/decorators/custom-element.js",
            "lit/decorators/property.js": "/assets/packages/lit/decorators/property.js",
            "lit/directives/class-map.js": "/assets/packages/lit/directives/class-map.js",
            "lit/directives/if-defined.js": "/assets/packages/lit/directives/if-defined.js",
            "lit/directives/repeat.js": "/assets/packages/lit/directives/repeat.js",
            "lit/static-html.js": "/assets/packages/lit/static-html.js",
            "tslib": "/assets/packages/tslib/tslib.es6.mjs"
          }
        }
      </script>
      <script type="module">
        import '/assets/javascript/dsd-polyfill.js';
        import '@lit-labs/ssr-client/lit-element-hydrate-support.js';
        import 'element-internals-polyfill';
        import '@rhds/elements/rh-card/rh-card.js';
        import '@rhds/elements/rh-cta/rh-cta.js';
      </script>
    </head>
    <body>
      <main>
        <rh-card color-palette="darkest">
          <rh-cta slot="footer" href="#"></rh-cta>
        </rh-card>
      </main>
    </body>
  </html>
`));

const result = render(tpl, { elementRenderers: [RHDSSSRableRenderer] });
const rendered = await collectResult(result);

await writeFile(
  new URL('./_site/repro.html', import.meta.url),
  trimOuterMarkers(rendered),
  'utf8',
);

@bennypowers
Copy link
Member Author

as of 9f048b1, context basically works (barring some additional qe, the inevitable small bugs etc)

However SSR is still fraught because the state of a SlotController can't easily be reset by client, leading to mismatches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

1 participant