-
Notifications
You must be signed in to change notification settings - Fork 168
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
[scoped-custom-element-registry] No support for isomorphic scripts #601
Comments
There's currently no platform support for scoped custom elements in DSD. You'd have to use a polyfilled version of DSD for this to work. Because of this, it seems like there's really limited value in making the polyfill work in Node at the moment. However, if you just want it to not produce an error, this seems reasonable and we'd welcome a PR for this. |
That's perfectly fine, all I'm looking to achieve is importing without throwing errors. I'll work on a PR! |
It's unclear why this code should be running on the server at all. Since this is a polyfill, it's intended use is for end users to add it to their application, not for individual elements to load and depend on it. |
So it's an issue of isomorphic code that can run on both server and client. When attempting to write a single definition for a web component that can SSR when no browser environment is detected, it becomes difficult to separate the static imports. To be clear, I agree, individual elements should not use the polyfill. But the issue is that all web components should be imported alongside the polyfill. In theory, the component classes could be imported in two separate scripts (client and server) but that becomes burdensome. IMHO, it would be more ideal to just import the polyfill without worrying too much about its behavior on the server. |
I think what we want here if for the server DOM shim to just implement scored registries, like it implements the global registry today. We don't want to load the scoped registry polyfill, just like we didn't load the custom elements or shadow DOM polyfill. Individual components didn't load those polyfills either, and server code didn't because they definitely don't run on the server. |
Sure, and yeah that's all my pull request achieves, essentially. When imported on the server, with this change, it will simply ignore the polyfill rather than crash the script. If I'm understanding what you mean, that is. |
What I'm saying is that it should crash. First, no one should be even trying to load this module on the server. The polyfill should be loaded by client-only app boot script that doesn't load on the server. Elements should definitely not be loading the polyfill themselves, they should leave it to the app. Second, components that incorrectly do load the polyfill will not work anyway. They're going to crash at some other point when they attempt to use APIs that aren't there. Maybe those elements feature detect the old, non-standard scoped registry APIs, but then they would work without the polyfill and shouldn't load it since it's optional. |
I'm not sure I really understand why it would be beneficial to exclude client+server apps. Maybe I'm not articulating clearly... the last time I tried a dynamic import with this polyfill, it did not work. I was unable to ascertain exactly why that was, at the time, but the point is it didn't work. So I'm not able to check for the window object myself and conditionally load this polyfill. It only works with a static import. It sounds like you're envisioning a clear separation of server scripts and client scripts, but that would require either: a. Some code-gen to automatically remove the import on the server only, or Perhaps I'm missing a third option that could make it easy to separate these cases, but I'm not sure I'm understanding how preventing a crash could harm the polyfill or its consumers, where a warning in the console would suffice. |
What if this was exported as a module instead? import { ScopedCustomElementRegistryPolyfill } from '@webcomponents/scoped-custom-element-registry';
// leave it to the consumer to load the script or not.
ScopedCustomElementRegistryPolyfill.load(); |
Just to be extra clear, here is my use case. I wrote some scripts that will either render HTML on the server or upgrade custom elements on the client, automatically, depending on the environment. import '@webcomponents/scoped-custom-element-registry';
import { MyButton } from './my-button';
import { MyCard } from './my-card';
import { MyInput } from './my-input';
MyButton.define('my-button');
MyCard.define('my-card');
MyInout.define('my-input'); First, this script runs on the server. The behavior of Then, the same script runs on the client. The behavior of On the client, this polyfill is needed, so it must be imported. Since the same code also runs on the server, the polyfill crashes the script. Hopefully the specifics of my problem are clearer with this. |
import '@webcomponents/scoped-custom-element-registry'; Right, this isn't the intended usage of the polyfill. Elements shouldn't import it since it changes the global environment. The containing page should import it. Then it should be relatively simple not to have it run on the server? |
I mean, again I agree, elements shouldn't import the polyfill. I guess I'm confused because the given code snippet is not an element, so I'm not sure how to respond to that. All elements are imported and defined once, you could consider this snippet to be the containing page. |
Sorry, I should have been more clear, and I understand needs in practice can vary so I don't mean to say you're doing anything explicitly wrong. That said, the idea I was getting at with the polyfill is that it is explicitly loaded in isolation from anything else. This makes it an explicit choice to load and ensures it's relatively simple to remove when no longer needed. And If that is done, then making an entry point that does not include it that is used for SSR is (hopefully) not too onerous. That's the hope at least and if we're clear that this is impractical, then we can work on a server-safe version. |
No worries, I feel like I'm just struggling to convey my meaning. If this is indeed an invalid usage of the polyfill and there's a better way around it, I'm all about changing the recommended approach here, I just need a working demo before I can document it. But I'm not yet convinced it's invalid, though. Let me step away and put more earnest thought into a reply, maybe create a sandbox code example to share the problem, and see if we can find a way to use the polyfill as-is. |
I actually might be reversing my stance on this. Let me get this proof of concept figured out, first. |
Okay @sorvell @justinfagnani, finally got around to proving this out in my local. I don't know why I didn't think about it before, but I just added the polyfill to an HTML script tag. I've got framework-brain, trying to solve everything in one isomorphic JS file. Thanks for your patience, I'll close this issue and the PR. |
edit: apologies @jonathandewitt-dev! I wrote this before refreshing the page, and I didn't see your latest response. Adding the polyfill script to the HTML is essentially the same as what I wrote here. Cheers! I think the missing part of the conversation might be how to have separate client and server entrypoints. I do it like:
/* client-only file */
import '@webcomponents/scoped-custom-element-registry';
import '@lit-labs/ssr-client/lit-element-hydrate-support.js';
import './components/app-component.ts'; // where index.html has <app-component>
/* server-only file */
export './components/app-component.ts'; // Used to call into SSR render() So no components or other code that runs on the server loads the polyfill. |
Description
I'm attempting to render DSD for web components on the server side. It's difficult to support scoped elements, however, because the polyfill is not server-friendly.
The polyfill must be statically imported before my code; conditionally loading the polyfill does not work.
Example
Simply import the library and launch the script using Node.
Expected behavior
No error should be thrown. Script should be ignored on the server.
Actual behavior
Version
0.0.9
The text was updated successfully, but these errors were encountered: