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: use globalThis as window fallback in client-window.ts #4916

Closed
3 tasks done
dwirz opened this issue Oct 9, 2023 · 10 comments
Closed
3 tasks done

feat: use globalThis as window fallback in client-window.ts #4916

dwirz opened this issue Oct 9, 2023 · 10 comments
Assignees

Comments

@dwirz
Copy link

dwirz commented Oct 9, 2023

Prerequisites

Describe the Feature Request

Use globalThis as window fallback in client-window.ts so that it gets possible to run it in an Node environment.

Describe the Use Case

In one of our client project we'd like to render stencil generated web components in a Next.js app directory application. With the help of luwes/wesc this is "possible" some stuff is still not working but the biggest change we need is that stencil is using globalThis so we can provide the necessary functions when the components are rendered server side in Node. (PR incoming)

Describe Preferred Solution

No response

Describe Alternatives

No response

Related Code

No response

Additional Information

No response

@rwaskiewicz
Copy link
Member

Hey @dwirz 👋

Thanks for the feature request + PR!

While the code that you provided in the PR seems safe, we'd like a little more information here. From our side, we don't quite understand why this change is necessary based on the information provided. We'd love the have the ability to reproduce the issue that you're seeing so that we can verify the fix works as intended. This is important for the long term maintenance of the code, so that we can understand why a change was made and to avoid regressions.

Can you do me a favor and provide a minimal reproduction case for the error you're seeing? From there, we can move forward with your PR.

Thanks

@rwaskiewicz rwaskiewicz added the ionitron: needs reproduction This PR or Issue does not have a reproduction case URL label Oct 10, 2023
@ionitron-bot
Copy link

ionitron-bot bot commented Oct 10, 2023

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Stencil starter component library and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@ionitron-bot ionitron-bot bot removed the triage label Oct 10, 2023
@rwaskiewicz rwaskiewicz added the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Oct 10, 2023
@dwirz
Copy link
Author

dwirz commented Oct 13, 2023

Hi @rwaskiewicz

I created a repo with a minimal reproduction description here.

Cheers

@ionitron-bot ionitron-bot bot removed the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Oct 13, 2023
@rwaskiewicz rwaskiewicz removed the ionitron: needs reproduction This PR or Issue does not have a reproduction case URL label Oct 18, 2023
@rwaskiewicz rwaskiewicz self-assigned this Oct 18, 2023
@rwaskiewicz
Copy link
Member

Thanks @dwirz !

I'm trying to ensure that I understand the reproduction/use-case here:

  • Is it the case that this Stencil library is using the WeSC library to wrap the components to use React, so that they can be used in a React/NextJS environment? If so:
    • Do the Stencil React Wrappers not fit your needs?
    • Can you help me understand what/why WeSC was chosen? Admittedly, I know nothing about the library itself.
  • How are these component wrappers being generated? I see there's a build script , but I'm not entirely sure what it does/why it does what id does
    • The compiler error related to this request seems to emanate from this directory, which is why I ask

@rwaskiewicz rwaskiewicz added Awaiting Reply This PR or Issue needs a reply from the original reporter. and removed triage labels Oct 18, 2023
@dwirz
Copy link
Author

dwirz commented Oct 19, 2023

Hey @rwaskiewicz

Is it the case that this Stencil library is using the WeSC library to wrap the components to use React, so that they can be used in a React/NextJS environment?

Yes, the WeSC wrapper component and server import makes it possible to render Web Components server side in a Node environment.

Do the Stencil React Wrappers not fit your needs?

Unfortunately no, since they create React class components, which are not allowed in React Server Components. And therefore are not future proof, for going forward.

Can you help me understand what/why WeSC was chosen?

I have chosen WeSC because it looked the most promising to solve SSR Web Components in the app directory of Next.js. See their Web Component based media-chrome player running within the app directory of Next.js. example code

How are these component wrappers being generated?

The original build script is also from the media-chrome guys. I adjusted it a little bit, to get it working with Stencil.js Web Components in the end it generates React components like:

'use client';

import React from 'react';
import { defineCustomElement } from '../components/abc-button.js';
import { toNativeProps } from './lib/utils.js';

if (!customElements.get('abc-button')) {
  defineCustomElement();
}

const AbcButton = React.forwardRef(({ children = [], ...props }, ref) =>
  React.createElement('abc-button', toNativeProps({ ...props, ref }), children),
);

export { AbcButton };

Beside that it generates appropriate types for these React components e.g.

import type { JSX } from 'abc-web-components'; // Import from the Stencil.js generated types of our Web Components

// ...

declare const AbcButton: React.ForwardRefExoticComponent<React.PropsWithChildren<Partial<EnumsToStringLiterals<JSX.AbcButton> & { slot: string }>> & React.RefAttributes<HTMLElement | undefined>>;

Additionally the postbuild command compiles the lib-folder from TypeScript to JS. This folder includes the server.ts and client.ts which contain the code that enables the rendering of the Stencil.js Web Components server side.

The server.ts mocks window functions onto the globalThis which are needed when the following code gets called:

export function AbcWrapper({ children }) {
  if (typeof window === 'undefined') {
    // eslint-disable-next-line import/no-unresolved
    return import('./render.js').then(({ render }) => render(resolve(children))); // <- This gets called if the Component is rendered server side
  }

  return children;
}

The render function calls the Web Component code which expects to be run in window context, which is not the case server side, but since we mocked all the functions on globalThis this will work – as long as Stencil.js falls back to globalThis instead of an empty object when window is not defined.


As a proposal to make my change more failsafe we could do something like:

export const win = typeof window !== 'undefined' ? window : ((globalThis.__MOCKED_WINDOW__ ? globalThis : {}) as Window);

Or check globalThis for a window property like globalThis.HTMLElement e.g.

export const win = typeof window !== 'undefined' ? window : ((globalThis.HTMLElement ? globalThis : {}) as Window);

This way as long as the property is not available it would fallback to the current empty object. Wdyt?

@ionitron-bot ionitron-bot bot removed the Awaiting Reply This PR or Issue needs a reply from the original reporter. label Oct 19, 2023
@thilohaas
Copy link

Hey @rwaskiewicz, how would you like to proceed with this pull request? Without this change its impossible to implement a clean server side rendering of stencil web components. We currently need to patch stencil in all consumer projects, in order to make our components SSR ready.

@dwirz
Copy link
Author

dwirz commented Oct 31, 2023

hey @rwaskiewicz just wanted to check in, how things going? Is it possible to get this merged somehow 🙂

@EriCreator
Copy link

@tanner-reits @alicewriteswrongs can you maybe take a look at this?
It's not possible to use exported stenciljs components in the app directory of NextJS without this change.

@rwaskiewicz
Copy link
Member

Sorry about this folks - this managed to slip through the cracks.

I'll take a look at @dwirz's comment tomorrow and provide some feedback

@rwaskiewicz
Copy link
Member

Hey folks,

Apologies for the lack of response until now - as I mentioned yesterday, unfortunately this slipped through the cracks and was ignored longer than it should have been.

The team and I discussed this feature request have come to the conclusion that a user-defined globalThis entry is not an appropriate fallback for window. Specifically, we're concerned about:

  1. globalThis can have it's properties redefined. Specifically, fields like document can be set with something like globalThis = null; globalThis ??= {document: 'hello world!'}. Stencil relies on several properties to be set on window and document. If a user-defined replacement is provided, and we don't have a great way in terms of developer experience to inform users what they need to provide. This mutability can also lead to tough to debug issues, as the same instance of globalThis is used when the win object is used.
  2. A feature like this can be (generally) difficult to support long term. While it's easy to add, it's difficult for us to test against. We currently have no plans to support WeSC components. Although that could change in the future, it's not high on our priority list.

With that in mind, I'm going to close this issue and the associated PR. Please feel free to reach out with any questions in this issue.

@rwaskiewicz rwaskiewicz closed this as not planned Won't fix, can't repro, duplicate, stale Nov 3, 2023
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 a pull request may close this issue.

4 participants