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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from Dec 2, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46a234d
fix (labs/ssr): move to enhanced-resolve for package exports support
43081j Oct 15, 2022
cb54a51
test (labs/ssr): add tests for package exports resolution
43081j Oct 19, 2022
f7f3fbc
add changeset
43081j Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fuzzy-gorillas-wait.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
'@lit-labs/ssr': minor
---

Module resolution within SSR now supports package exports (via `package.json`)
42 changes: 38 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions packages/labs/ssr/package.json
View file
Open in desktop
Expand Up @@ -134,9 +134,9 @@
"@open-wc/testing-karma": "^4.0.9",
"@types/command-line-args": "^5.0.0",
"@types/koa": "^2.0.49",
"@types/koa__router": "^8.0.2",
"@types/koa-cors": "*",
"@types/koa-static": "^4.0.1",
"@types/koa__router": "^8.0.2",
"@types/parse5": "^6.0.1",
"@types/resolve": "^1.20.2",
"@webcomponents/template-shadowroot": "^0.1.0",
Expand All @@ -151,12 +151,12 @@
"@lit-labs/ssr-client": "^1.0.0",
"@lit/reactive-element": "^1.4.0",
"@types/node": "^16.0.0",
"enhanced-resolve": "^5.10.0",
"lit": "^2.3.0",
"lit-element": "^3.1.0",
"lit-html": "^2.3.0",
"node-fetch": "^3.2.8",
"parse5": "^6.0.1",
"resolve": "^1.10.1"
"parse5": "^6.0.1"
},
"engines": {
"node": ">=13.9.0"
Expand Down
24 changes: 9 additions & 15 deletions packages/labs/ssr/src/lib/module-loader.ts
View file
Open in desktop
Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Collaborator Author

@43081j 43081j Oct 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import {builtinModules} from 'module';

type PackageJSON = {main?: string; module?: string; 'jsnext:main'?: string};

const builtIns = new Set(builtinModules);

const specifierMatches = (specifier: string, match: string) =>
Expand Down Expand Up @@ -276,17 +274,11 @@ export const resolveSpecifier = async (
// a single version.
referrerPath = fileURLToPath(import.meta.url);
}
const modulePath = await resolve(specifier, {
basedir: path.dirname(referrerPath),
moduleDirectory: ['node_modules'],
const modulePath = await resolve(specifier, path.dirname(referrerPath), {
modules: ['node_modules'],
extensions: ['.js'],
// Some packages use a non-standard alternative to the "main" field
// in their package.json to differentiate their ES module version.
packageFilter: (packageJson: PackageJSON) => {
packageJson.main =
packageJson.module ?? packageJson['jsnext:main'] ?? packageJson.main;
return packageJson;
},
mainFields: ['module', 'jsnext:main', 'main'],
conditionNames: ['node'],
});
return pathToFileURL(modulePath);
}
Expand All @@ -301,10 +293,12 @@ const initializeImportMeta = (meta: {url: string}, module: vm.Module) => {

const resolve = async (
id: string,
opts: resolveAsync.AsyncOpts
path: string,
opts: Partial<enhancedResolve.ResolveOptions>
): Promise<string> => {
const resolver = enhancedResolve.create(opts);
43081j marked this conversation as resolved.
Show resolved Hide resolved
return new Promise((res, rej) => {
resolveAsync(id, opts, (err, resolved) => {
resolver({}, path, id, {}, (err: unknown, resolved?: string) => {
if (err != null) {
rej(err);
} else {
Expand Down
34 changes: 34 additions & 0 deletions packages/labs/ssr/src/test/lib/module-loader_test.ts
View file
Open in desktop
Expand Up @@ -53,4 +53,38 @@ test('loads a module with a built-in import', async () => {
assert.ok(module.namespace.join);
});

test('resolves an exact exported path', async () => {
const loader = new ModuleLoader({global: window});
const result = await loader.importModule('./lit-import.js', testIndex);
const {module, path: modulePath} = result;
assert.is(module.namespace.litIsServer, true);
assert.ok(loader.cache.has(modulePath));
const isServerPath = path.resolve(
path.dirname(testIndex),
'../../../../../lit-html/node/is-server.js'
);
assert.ok(loader.cache.has(isServerPath));
});

test('resolves a root exported path (.)', async () => {
const loader = new ModuleLoader({global: window});
const result = await loader.importModule(
'./lit-import-from-root.js',
testIndex
);
const {module, path: modulePath} = result;
assert.is(module.namespace.litIsServer, true);
assert.ok(loader.cache.has(modulePath));
const litPath = path.resolve(
path.dirname(testIndex),
'../../../../../lit/index.js'
);
const isServerPath = path.resolve(
path.dirname(testIndex),
'../../../../../lit-html/node/is-server.js'
);
assert.ok(loader.cache.has(litPath));
assert.ok(loader.cache.has(isServerPath));
});

test.run();
2 changes: 2 additions & 0 deletions packages/labs/ssr/src/test/test-files/module-loader/lit-import-from-root.js
View file
Open in desktop
@@ -0,0 +1,2 @@
import {isServer} from 'lit';
export const litIsServer = isServer;
2 changes: 2 additions & 0 deletions packages/labs/ssr/src/test/test-files/module-loader/lit-import.js
View file
Open in desktop
@@ -0,0 +1,2 @@
import {isServer} from 'lit-html/is-server.js';
export const litIsServer = isServer;