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

[lit] reexport of html from lit fails because of side effects? #2524

Closed
daKmoR opened this issue Feb 11, 2022 · 16 comments
Closed

[lit] reexport of html from lit fails because of side effects? #2524

daKmoR opened this issue Feb 11, 2022 · 16 comments
Assignees

Comments

@daKmoR
Copy link
Contributor

daKmoR commented Feb 11, 2022

Description

I would expect to have a file with

export { html } from 'lit';

to also work if I execute it in node...

Steps to Reproduce

  1. Open Stackblitz demo https://stackblitz.com/edit/node-uqy5mf?file=index.js
  2. execute node index.js in the stackblitz terminal

Expected Results

A console log

Actual Results

❯ node index.js
Error: document is not defined
...

seems a side effect is doing something and fails

@justinfagnani
Copy link
Collaborator

Is this about SSR?

@daKmoR
Copy link
Contributor Author

daKmoR commented Feb 11, 2022

sort of... consider something like this

// page.js
import { html } from 'lit';

export const content = () => html`<p>...</p>`;

export const foo = 'bar';

now I would like to get foo in a node process.
I would assume something like this to work

import { foo } from './page.js';

I would even assume this to work

import { foo, content } from './page.js';

I am fully aware that I won't be able to execute content() as that would "run" the function...

but in fact import { foo } from './page.js'; will already throw with the document error 😅

@justinfagnani
Copy link
Collaborator

The core modules definitely require some kind of DOM shim in order to load.

@daKmoR
Copy link
Contributor Author

daKmoR commented Feb 11, 2022

this will run in node with the polyfills for ssr and will work fine

import { content } from './page.js';

ssrRender(content());

this will run in "pure" node and will error...

import { foo } from './page.js';

// encode and write foo to a file

I mean I can always import the polyfills, even though I don't use it in that case... but that feels dirty 😭

The core modules definitely require some kind of DOM shim in order to load.

why? I do not execute any of the functions... 🤔

@daKmoR
Copy link
Contributor Author

daKmoR commented Feb 23, 2022

the plot thickens... I now need to add those two import at the top to any file that at some point imports lit-html

// we load this before the global-dom-shim as otherwise prism thinks it's running in a browser 🙈
import 'rehype-prism-template';
// we need to load the global-dom-shim as otherwise import { html } from 'lit'; breaks
import '@lit-labs/ssr/lib/install-global-dom-shim.js';

works - but feels quite wrong

@aomarks
Copy link
Member

aomarks commented Jul 25, 2022

We've just published some pre-releases for Lit which includes #3156. This change means that Lit should now be importable directly from Node, without crashing, without the need to first load the global DOM shim.

You should be able to upgrade with:

npm i lit@^2.3.0-next.1

Note this won't automatically enable any kind of actual SSR of Lit components out-of-the-box, since that requires specific integration with the framework/renderer -- but it should allow you to import without crashing Node.

Also note that this does not automatically globally define any DOM APIs like the global DOM shim does, apart from customElements which is shimmed as a no-op. So if you have any calls like window.addEventListener running at module initialization, that will still error.

If you have a chance to check out this pre-release, that would be great, and please comment here to let us know how that works for you!

@bugwheels94
Copy link

@aomarks Amazing! I tried to use inside Nextjs this as soon as I saw but it broke on different error for me

../node_modules/@lit/reactive-element/node/css-tag.js (6:1220) @ c
ReferenceError: CSSStyleSheet is not defined

This import is coming from customElement()

customElement(lit/decorator.js) -> ClassDescriptor (base.js) -> Reactive Element(reactive-element.js) -> css-tag.js

Thanks for this feature.

@daKmoR
Copy link
Contributor Author

daKmoR commented Jul 27, 2022

sweet - for the simple test case

👉 index.js

console.log(`Hello from Node.js v${process.versions.node}!`);
export { html, css, LitElement, svg, unsafeCSS, nothing } from "lit";
console.log("✅ could export html via node");
node index.js

it works fine 💪

will check for Rocket tomorrow but I assume it will work fine 🤗

@aomarks
Copy link
Member

aomarks commented Jul 27, 2022

@aomarks Amazing! I tried to use inside Nextjs this as soon as I saw but it broke on different error for me

../node_modules/@lit/reactive-element/node/css-tag.js (6:1220) @ c
ReferenceError: CSSStyleSheet is not defined

This import is coming from customElement()

customElement(lit/decorator.js) -> ClassDescriptor (base.js) -> Reactive Element(reactive-element.js) -> css-tag.js

Thanks for this feature.

Thanks for the report @bugwheels94. Can you provide a little more context? What does your element definition look like? Are you using Lit SSR?

@bugwheels94
Copy link

@aomarks Here is the file that I was trying to import in nextjs

File https://jsfiddle.net/prh5y4Lb/1/

This file is generated using rollup. The error comes in line 43 when using customElements.

No, I am not using Lit SSR. I am just using Nextjs

@aomarks
Copy link
Member

aomarks commented Jul 29, 2022

This file is generated using rollup. The error comes in line 43 when using customElements.

@bugwheels94 Ah I see. Rollup is going to select the default export condition by default. You'll want to set exportConditions: ["node"] in your nodeResolve settings to get the Node-specific build instead: https://github.com/rollup/plugins/tree/master/packages/node-resolve/#exportconditions. We don't include the required shims in the browser build, because we don't want to pay that byte cost in the browser.

Doesn't nextjs do its own bundling? I'm curious why you are bundling with rollup yourself? (I'm also quite new to nextjs).

Ideally you would use the node export condition for the code that is running on the server-side, and the default export condition for the code that is running in the browser. I believe that nextjs would do this by default.

@bugwheels94
Copy link

bugwheels94 commented Jul 30, 2022

@aomarks So, we are making a UI library using WC with Lit. We have many things like Linaria(CSS) with custom roolup plugins(for using linaria with constructed css), React portal like event proxying for actual overlays so we made it separate repo for many repos to use at once. We are planning to get it opensource as it is behind enterprise right now.

Since lit is our external deps and i dont bundle it as shown in the file. I think nextjs should not have any problem parsing browser/main field by default. i will check deeper and will update here for sure.

This is a bit new thing for ui so mentioning will help many like me in future. Thanks!

This will change saved me so much trouble of dynamic import and all in Nextjs. Many thanks!

@daKmoR
Copy link
Contributor Author

daKmoR commented Aug 11, 2022

after some cleanup/fight with prism I can now confirm that this version 2.3.0-next.1 allows me to remove all the "nasty" workaround I have in Rocket for it 💪 - nice 🤗

any timeline for when this will be released? 😬

@aomarks
Copy link
Member

aomarks commented Aug 11, 2022

after some cleanup/fight with prism I can now confirm that this version 2.3.0-next.1 allows me to remove all the "nasty" workaround I have in Rocket for it muscle - nice hugs

Awesome! Thanks for checking it out.

any timeline for when this will be released? grimacing

Later today!

@aomarks
Copy link
Member

aomarks commented Aug 12, 2022

any timeline for when this will be released? grimacing

Later today!

Just released!

https://unpkg.com/browse/lit-html@2.3.0/node/

@daKmoR
Copy link
Contributor Author

daKmoR commented Aug 13, 2022

Updated and used in rocket now 🎉

Sooo nice to get rid of this "workaround" 💪
thxxx 🙇‍♂️

In case anyone is curious what it helped to clean up
modernweb-dev/rocket#398

@daKmoR daKmoR closed this as completed Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants