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.

Sign up for GitHub

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

Jump to bottom

fix: prevent vitetest/ssr errors due to defining components on the server #7521

Merged
merged 4 commits into from
Aug 15, 2023
Merged

fix: prevent vitetest/ssr errors due to defining components on the server #7521

merged 4 commits into from
Aug 15, 2023

Conversation

benelan
Copy link
Member

@benelan benelan commented Aug 15, 2023 •
edited by jcfranco
Loading

Related Issue: #7486

Summary

Enabling Stencil's includeImportCustomElements option prevented SSR and vitetest users from being able to explicitly define the custom elements on the client. This patches the Stencil build to ensure the custom elements aren't defined while on the server.

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Aug 15, 2023
@benelan benelan added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Aug 15, 2023
jcfranco
jcfranco approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Minor comments, but this LGTM! 🎉

packages/calcite-components-react/src/auto-define.ts Outdated Show resolved Hide resolved
packages/calcite-components-react/src/auto-define.ts
@@ -0,0 +1,12 @@
const isBrowser = (): boolean =>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion (minor): The result of this can be computed earlier or cached. Multiple autoDefine calls run the same logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Per our Teams conversation, we decided to leave this as is for now just to be very safe.

It is possible (but unlikely) that if we change isBrowser to a constant its could be cached on the server as false by an SSR framework trying to be too smart. If this were to happen and the constant didn't get reevaluated on the client, the components would never be defined.

This needs a bit more investigation.

packages/calcite-components-react/support/patchSSR.ts Outdated Show resolved Hide resolved
@benelan benelan added skip visual snapshots Pull requests that do not need visual regression testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 15, 2023
@benelan
Copy link
Member Author

benelan commented Aug 15, 2023 •
edited
Loading

Chromatic snapshots looked good on a previous build so I skipped them because they don't need to be ran again.

@benelan benelan merged commit 046672e into main Aug 15, 2023
@benelan benelan deleted the benelan/auto-define-ccr-patch branch August 15, 2023 01:38
@github-actions github-actions bot added this to the 2023 August Priorities milestone Aug 15, 2023
@calcite-admin calcite-admin mentioned this pull request Aug 15, 2023
Merged
benelan pushed a commit that referenced this pull request Aug 15, 2023
@calcite-admin @github-actions
chore: release main (#7485)
124df0a 
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-components: 1.6.0</summary>

##
[1.6.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@1.5.1...@esri/calcite-components@1.6.0)
(2023-08-15)


### Features

* **action-bar:** Add "actions-end" slot (deprecates "bottom-actions")
([#7435](#7435))
([1bf14ff](1bf14ff))


### Bug Fixes

* **block:** Defaults the status icon to `scale=s`
([#7503](#7503))
([e1aee99](e1aee99))
* **button,card,fab,inline-editable:** Provides context to AT users when
loading
([#7257](#7257))
([df33eda](df33eda))
* **chip-group:** Add existence checks
([#7487](#7487))
([33225a7](33225a7))
* **combobox:** Prevents navigation list with Space key
([#7505](#7505))
([58e2ff2](58e2ff2))
* **panel:** Fix heading border when only text content is slotted
([#7491](#7491))
([7704400](7704400))
* **progress:** Completes animation for `dir='rtl'`
([#7511](#7511))
([c5d6ada](c5d6ada))
* **scrim:** Handle slotted children correctly
([#7477](#7477))
([c5ce008](c5ce008))
* **scrim:** Render text content inside scrim
([#7509](#7509))
([643ce5d](643ce5d))
* **slider:** Rerender ticks when prop is modified
([#7439](#7439))
([20058a9](20058a9))
* **tree:** Selects all child items when selection-mode is set to
ancestors
([#7518](#7518))
([f1eef84](f1eef84))
</details>

<details><summary>@esri/calcite-components-react: 1.6.0</summary>

##
[1.6.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@1.5.1...@esri/calcite-components-react@1.6.0)
(2023-08-15)


### Bug Fixes

* Prevent vitetest/ssr errors due to defining components on the server
([#7521](#7521))
([046672e](046672e))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^1.6.0-next.7 to ^1.6.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
benelan pushed a commit that referenced this pull request Aug 16, 2023
@calcite-admin @github-actions @benelan
chore: release main (#7485)
ff8ba0f 
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-components: 1.6.0</summary>

##
[1.6.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@1.5.1...@esri/calcite-components@1.6.0)
(2023-08-15)


### Features

* **action-bar:** Add "actions-end" slot (deprecates "bottom-actions")
([#7435](#7435))
([1bf14ff](1bf14ff))


### Bug Fixes

* **block:** Defaults the status icon to `scale=s`
([#7503](#7503))
([e1aee99](e1aee99))
* **button,card,fab,inline-editable:** Provides context to AT users when
loading
([#7257](#7257))
([df33eda](df33eda))
* **chip-group:** Add existence checks
([#7487](#7487))
([33225a7](33225a7))
* **combobox:** Prevents navigation list with Space key
([#7505](#7505))
([58e2ff2](58e2ff2))
* **panel:** Fix heading border when only text content is slotted
([#7491](#7491))
([7704400](7704400))
* **progress:** Completes animation for `dir='rtl'`
([#7511](#7511))
([c5d6ada](c5d6ada))
* **scrim:** Handle slotted children correctly
([#7477](#7477))
([c5ce008](c5ce008))
* **scrim:** Render text content inside scrim
([#7509](#7509))
([643ce5d](643ce5d))
* **slider:** Rerender ticks when prop is modified
([#7439](#7439))
([20058a9](20058a9))
* **tree:** Selects all child items when selection-mode is set to
ancestors
([#7518](#7518))
([f1eef84](f1eef84))
</details>

<details><summary>@esri/calcite-components-react: 1.6.0</summary>

##
[1.6.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@1.5.1...@esri/calcite-components-react@1.6.0)
(2023-08-15)


### Bug Fixes

* Prevent vitetest/ssr errors due to defining components on the server
([#7521](#7521))
([046672e](046672e))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^1.6.0-next.7 to ^1.6.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@benelan benelan mentioned this pull request Oct 26, 2023
Merged
benelan added a commit to Esri/calcite-components-examples that referenced this pull request Oct 26, 2023
@benelan
fix(react): add back CC imports due to potential stencil bug (#269)
77adb77 
There is an issue with auto-importing and defining calcite components
using the react wrapper via
Esri/calcite-design-system#7185

It appears to be a Stencil bug related to our SSR patch which
dynamically imports the web components to prevent them from rendering on
the server.
Esri/calcite-design-system#7521
@benelan benelan mentioned this pull request Nov 8, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

@driskull driskull Awaiting requested review from driskull

@geospatialem geospatialem Awaiting requested review from geospatialem

Assignees

@benelan benelan

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Milestone
1.6.0 - Low risk issues only
Development

Successfully merging this pull request may close these issues.
3 participants
@benelan @jcfranco @geospatialem