disabled shadydom in node dev build

[WickyNilliams]

In a next.js environent this line was throwing because window is undefined. This module was imported transitively via a component importing the ref directive, which in turn imports async-directive, which in turn imports the file where the fix was added.

Related #3156

I notice there are tests for lit-html in that PR which import the ref directive, so i'm not sure how they pass (or conversely how it is not working for me)

[changeset-bot]

🦋 Changeset detected

Latest commit: 87754c1

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

This PR includes changesets to release 2 packages

Name	Type
lit	Patch
lit-html	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

[augustjk]

Thank you for the PR!

Ah.. this probably only happens when running in dev mode in Next.js and introduced with #3495. The tests you reference are passing because they are testing against the production Node build in which ENABLE_SHADYDOM_NOPATCH is replaced with false during the rollup build.

It makes no sense to keep that true in Node builds regardless of prod/dev, so I think the correct move might be to make that false for dev Node builds as well.

The lines here

lit/rollup-common.js

Lines 452 to 453 in feb2494

	'const ENABLE_SHADYDOM_NOPATCH = true':
	'const ENABLE_SHADYDOM_NOPATCH = false',

should also be added below here

lit/rollup-common.js

Line 496 in feb2494

'const NODE_MODE = false': 'const NODE_MODE = true',

[WickyNilliams]

excuse the delay, first day back in the office. i'll update the PR as suggested, and also generate a changeset

[WickyNilliams]

ok, updated as per suggestion and dropped previous commit.

re: changesets, should i just pick the lit package? i am wondering if this change is more broad since it changes the common rollup config

[augustjk]

re: changesets, should i just pick the lit package? i am wondering if this change is more broad since it changes the common rollup config

I think a patch to lit and lit-html makes sense as the ones being affected by that flag.

Thank you!

[WickyNilliams]

added the changeset as recommended. i had to battle with the CLA check a little, but it's all good now.

[augustjk]

Thank you very much!
I added a bit more description the changeset and tests for node dev builds.

[WickyNilliams]

Great, thanks! Appreciate you taking the time