fix (labs/ssr): move to enhanced-resolve for package exports support #3367

43081j · 2022-10-15T09:46:29Z

The resolve package doesn't support package exports and various other more modern resolution features. It seems enhanced-resolve (from webpack) introduces this support.

Fixes #3270

@justinfagnani can you or someone on the team help me figure out the right way to add a test for this? the current module loader tests don't test bare specifiers it seems, so i don't have any examples to work from.

until then im not 100% sure it fixes 3270, so do want to get that added

changeset-bot · 2022-10-15T09:46:32Z

🦋 Changeset detected

Latest commit: f7f3fbc

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

This PR includes changesets to release 1 package

Name	Type
@lit-labs/ssr	Minor

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

aomarks

Awesome, thanks for this PR @43081j!

aomarks · 2022-10-17T22:14:28Z

packages/labs/ssr/src/lib/module-loader.ts

@@ -8,11 +8,9 @@ import * as path from 'path';
 import {promises as fs} from 'fs';
 import {URL, fileURLToPath, pathToFileURL} from 'url';
 import * as vm from 'vm';
-import resolveAsync from 'resolve';
+import enhancedResolve from 'enhanced-resolve';


Did you look at https://github.com/lukeed/resolve.exports as an alternative to https://github.com/webpack/enhanced-resolve? I haven't evaluated them, but one thing at least that's nice about resolve.exports is that it has a js modules version in the package. enhanced-resolve only seems to distribute commonjs. It would be nice to try and stay on the js module track for SSR dependencies, to maximize the possibility for e.g. running in browser down the line.

i wasn't aware of it, but i do agree it'd be nice to have an ESM dependency.

it seems though, that package is "tiny" because it only deals with resolution on a per-package basis. enhanced-resolve handles the filesystem traversal for us, which we'd otherwise have to implement.

so we could use resolve.exports but we'd have to do a fair chunk of extra code to handle the actual traversal

Sorry just saw this; I had needed to add support for export conditions on some of my analyzer exploration work this summer, and resolve.exports just dropped in nicely into the pathFilter arg of our existing resolve: https://github.com/lit/lit/compare/analyzer-pull/#diff-e8a74852b15cd69316ce54daa6fde47bc03b4a8eee3d643bc45546106e65a9c6

Haven't looked deeper into the pros/cons of either approach, but seemed to "just work".

That would work but keep in mind it wouldn't solve what al is talking about afaict.

Both resolve and enhanced-resolve are shipped as commonjs, so with either of these solutions we end up with a commonjs dependency. Though it's already that way so maybe a problem for another pr.

In that case though I would be tempted to rely on one package for the resolution rather than two.

If we built our own filesystem traversal so we could drop the resolve package, I'd choose resolve.exports.

Otherwise if resolve or enhanced-resolve started shipping esm, I'd avoid duplication and just use the one package

resolve.exports does already ship esm: https://unpkg.com/browse/resolve.exports@1.1.0/dist/index.mjs

Oh, maybe I misunderstood; looks like you need to pass a base resolve to resolve.exports, so it's literally the resolve package you are saying is commonjs.

yes exactly

resolve and enhanced-resolve are both commonjs.

resolve.exports doesn't implement path resolution, it just implements understanding of the package manifest.

so even if we use it, we need to still use one of the other 2 packages to do the actual filesystem traversal

packages/labs/ssr/src/lib/module-loader.ts

43081j · 2022-11-10T17:39:35Z

@aomarks @kevinpschaaf any chance you guys can help me move this one along? guess we need to decide if we use enhanced-resolve or (resolve + resolve.exports) - 1 package vs 2

aomarks · 2022-11-10T19:23:09Z

@aomarks @kevinpschaaf any chance you guys can help me move this one along? guess we need to decide if we use enhanced-resolve or (resolve + resolve.exports) - 1 package vs 2

Sorry for the delay. Since both add a commonjs dependency, I'm totally fine with enhanced-resolve.

Can you add a changeset? (npx changeset)

43081j · 2022-11-10T21:34:10Z

@aomarks no worries, have added a changeset. let me know if all is ok

i saw the tests failed before too, if you can trigger the run again ill look into those

aomarks

LGTM assuming tests pass

43081j · 2022-11-10T22:35:52Z

looks like the test failure is this:

FAIL "template files with permalink: false in frontmatter"
11ty process didn't exit in 10000ms. (not)

not too sure whats going on there.

the windows tests are having a horrid time too:

Error: src/commands.ts(23,8): error TS2307: Cannot find module '@lit/localize-tools/lib/config.js' or its corresponding type declarations.
Error: src/commands.ts(24,37): error TS2307: Cannot find module '@lit/localize-tools/lib/modes/transform.js' or its corresponding type declarations.
Error: src/commands.ts(25,35): error TS2307: Cannot find module '@lit/localize-tools/lib/modes/runtime.js' or its corresponding type declarations.
Error: src/commands.ts(26,39): error TS2307: Cannot find module '@lit/localize-tools/lib/error.js' or its corresponding type declarations.
Error: src/commands.ts([27](https://github.com/lit/lit/actions/runs/3440577952/jobs/5739762179#step:6:28),28): error TS2307: Cannot find module '@lit/localize-tools/lib/index.js' or its corresponding type declarations.
Error: src/commands.ts([28](https://github.com/lit/lit/actions/runs/3440577952/jobs/5739762179#step:6:29),32): error TS2[30](https://github.com/lit/lit/actions/runs/3440577952/jobs/5739762179#step:6:31)7: Cannot find module '@lit/localize-tools/lib/typescript.js' or its corresponding type declarations.

The resolve package doesn't support package exports and various other more modern resolution features. It seems `enhanced-resolve` (from webpack) introduces this support.

43081j · 2022-11-27T22:28:38Z

@aomarks i've rebased onto latest main

any chance you could trigger CI so i can see if those tests pass now?

augustjk

Looks like tests are passing!

43081j requested a review from justinfagnani as a code owner October 15, 2022 09:46

aomarks requested changes Oct 17, 2022

View reviewed changes

43081j force-pushed the super-resolver branch from 4b83ea5 to 2e13a38 Compare October 19, 2022 21:39

43081j force-pushed the super-resolver branch from 2e13a38 to 7c2b15a Compare November 10, 2022 21:32

aomarks approved these changes Nov 10, 2022

View reviewed changes

43081j added 3 commits November 27, 2022 22:27

fix (labs/ssr): move to enhanced-resolve for package exports support

46a234d

The resolve package doesn't support package exports and various other more modern resolution features. It seems `enhanced-resolve` (from webpack) introduces this support.

test (labs/ssr): add tests for package exports resolution

cb54a51

add changeset

f7f3fbc

43081j force-pushed the super-resolver branch from 7c2b15a to f7f3fbc Compare November 27, 2022 22:28

augustjk approved these changes Dec 2, 2022

View reviewed changes

augustjk merged commit b04c11b into lit:main Dec 2, 2022

43081j deleted the super-resolver branch December 2, 2022 22:36

augustjk mentioned this pull request Dec 2, 2022

feat(labs/ssr): Prevent exposure of window object on globalThis #3431

Merged

This was referenced Dec 7, 2022

Version Packages #3504

Closed

Version Packages #3506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix (labs/ssr): move to enhanced-resolve for package exports support #3367

fix (labs/ssr): move to enhanced-resolve for package exports support #3367

43081j commented Oct 15, 2022 •

edited

changeset-bot bot commented Oct 15, 2022 •

edited

aomarks left a comment

aomarks Oct 17, 2022

43081j Oct 18, 2022

kevinpschaaf Oct 19, 2022

43081j Oct 19, 2022 •

edited

aomarks Oct 19, 2022

aomarks Oct 19, 2022

43081j Oct 19, 2022

43081j commented Nov 10, 2022

aomarks commented Nov 10, 2022

43081j commented Nov 10, 2022

aomarks left a comment

43081j commented Nov 10, 2022

43081j commented Nov 27, 2022

augustjk left a comment

fix (labs/ssr): move to enhanced-resolve for package exports support #3367

fix (labs/ssr): move to enhanced-resolve for package exports support #3367

Conversation

43081j commented Oct 15, 2022 • edited

changeset-bot bot commented Oct 15, 2022 • edited

🦋 Changeset detected

aomarks left a comment

Choose a reason for hiding this comment

aomarks Oct 17, 2022

Choose a reason for hiding this comment

43081j Oct 18, 2022

Choose a reason for hiding this comment

kevinpschaaf Oct 19, 2022

Choose a reason for hiding this comment

43081j Oct 19, 2022 • edited

Choose a reason for hiding this comment

aomarks Oct 19, 2022

Choose a reason for hiding this comment

aomarks Oct 19, 2022

Choose a reason for hiding this comment

43081j Oct 19, 2022

Choose a reason for hiding this comment

43081j commented Nov 10, 2022

aomarks commented Nov 10, 2022

43081j commented Nov 10, 2022

aomarks left a comment

Choose a reason for hiding this comment

43081j commented Nov 10, 2022

43081j commented Nov 27, 2022

augustjk left a comment

Choose a reason for hiding this comment

43081j commented Oct 15, 2022 •

edited

changeset-bot bot commented Oct 15, 2022 •

edited

43081j Oct 19, 2022 •

edited