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(core): ssr events #2891

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

fix(core): ssr events #2891

wants to merge 15 commits into from

Conversation

bennypowers
Copy link
Member

@bennypowers bennypowers commented Jan 13, 2025

The ## What I did

  1. update lit packages related to ssr and context (i.e. events in ssr)
  2. write a server-side SlotController which adds hint attributes to all elements that use SlotController. deferred to feat(core): slot controller ssr hint attributes #2893

Testing Instructions

  1. view SSR test output, there should be no errors related to addEventListener

Notes to Reviewers

  1. see [docs] Add documentation for SSR event handling lit/lit.dev#1390 and [labs/ssr] Implement SSR custom elements event handling lit/lit#4755
  2. there are some element refactors here which account for the fact that we're opting in to calling connectedCallback and hostConnected - i suspect some of these should be refactored in order to make that more clear from the code
  3. netlify builds are failing, probably because there are too many files. that will have to be dealt with somehow

Copy link

changeset-bot bot commented Jan 13, 2025

🦋 Changeset detected

Latest commit: 453280b

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

This PR includes changesets to release 2 packages
Name Type
@patternfly/pfe-core Patch
@patternfly/elements 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

@bennypowers bennypowers changed the title Fix/ssr events fix(core): ssr events Jan 13, 2025
Copy link
Contributor

github-actions bot commented Jan 13, 2025

✅ Commitlint tests passed!

More Info
{
  "valid": true,
  "errors": [],
  "warnings": [],
  "input": "fix(core): ssr events"
}

Copy link

netlify bot commented Jan 13, 2025

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 453280b
🔍 Latest deploy log https://app.netlify.com/sites/patternfly-elements/deploys/679baa36068e70000882a040
😎 Deploy Preview https://deploy-preview-2891--patternfly-elements.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.

@github-actions github-actions bot added the AT passed Automated testing has passed label Jan 13, 2025
Copy link
Contributor

SSR Test Run for 2877a4b: Report

Copy link
Contributor

SSR Test Run for 5c9ba1c: Report

Copy link
Contributor

SSR Test Run for 09809cd: Report

bennypowers added a commit to RedHat-UX/red-hat-design-system that referenced this pull request Jan 19, 2025

;
// @ts-expect-error: opt in to event support in ssr
globalThis.litSsrCallConnectedCallback = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

should we really be making this decision for our users? or should e.g. rhds' ssr regime choose whether or not to enable this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having a hard time trying to figure out if this is hostile in any way. Maybe I'm misunderstanding, but doing otherwise would mean that we'd need to wrap all addEventListeners in !isServer() in component connectedCallback() correct? Could there be a helper method that replaces isServer() and includes the check to see if the litSsrCallConnectedCallback is enabled on the server while in SSR and otherwise falls back to normal isServer()?

Copy link
Contributor

SSR Test Run for 59bc1ae: Report

Copy link
Contributor

SSR Test Run for 71a6a7c: Report

Copy link
Member Author

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

This PR should be pared down to concentrate on lit, context-api, and table changes

slot controller changes should be deferred to another PR

Copy link
Contributor

SSR Test Run for cc5dc4d: Report

This will be addressed in a later PR
Copy link
Contributor

SSR Test Run for 601414b: Report

Copy link
Contributor

SSR Test Run for c1179a7: Report

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe move these changes to a new PR since they are less concerned with the SSR changes and are a fix for pf-table explicitly, although the context change allows it to happen in SSR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AT passed Automated testing has passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants