simplify package path resolution #8186

turadg · 2023-08-14T15:39:37Z

Description

import.meta.resolve returns a string now instead of a promise. https://github.com/wooorm/import-meta-resolve/releases/tag/3.0.0 updates the ponyfill.

This updates to it and goes on to simplify getting paths from packages.

Before

  const autoswapContractUrl = await importMetaResolve(
    '@agoric/zoe/src/contracts/autoswap.js',
    import.meta.url,
  );
  const autoswapContractRoot = new URL(autoswapContractUrl).pathname;

After

  const autoswapContractRoot = pkgAbsPath(
    '@agoric/zoe/src/contracts/autoswap.js',
  );

Where pkgAbsPath is imported from a utility module. It has its own import.meta.url, which is sufficient for resolving packages.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc

For resolving paths that are known at design-time, I'm fine with sync access.

But for paths that vary at runtime, let's not dip into sync I/O any more than we must.

dckc · 2023-08-15T21:46:31Z

packages/SwingSet/src/controller/initializeSwingset.js

- * @returns {Promise<string>} the absolute path corresponding to `specPath` if it can be
+ * @returns {string} the absolute path corresponding to `specPath` if it can be


This is a case where the path is dynamic - not determined until runtime. Let's keep it async, please.

Using sync I/O for a sort of linker/loader makes sense, but otherwise, let's not force clients into the world of sync I/O.

dckc · 2023-08-15T21:47:03Z

packages/SwingSet/src/controller/initializeSwingset.js

@@ -195,31 +194,27 @@ async function resolveSpecFromConfig(referrer, specPath) {
 * @param {boolean} expectParameters `true` if the entries should have parameters (for
 *    example, `true` for `vats` but `false` for bundles).
 */
-async function normalizeConfigDescriptor(desc, referrer, expectParameters) {


dckc · 2023-08-15T21:48:21Z

packages/SwingSet/tools/paths.js

+ * @param {string} specifier
+ * @returns {string} file system absolute path of the package resource specifier
+ */
+export const pkgAbsPath = specifier => {


It's not clear that this is only used with paths that are known at design-time. Let's keep runtime I/O async.

dckc · 2023-08-15T21:50:34Z

packages/boot/test/bootstrapTests/supports.js

-  const fullPath = await importMetaResolve(specifier, import.meta.url).then(
-    u => new URL(u).pathname,
-  );
+  const fullPath = pkgAbsPath(specifier);


presuming the specifier arg is always a design-time constant, sync access is OK here. I'd be OK with copying pkgAbsPath into this module.

My preference is to inline url.fileURLToPath(importMetaResolve(specifier, import.meta.url)) here with the expectation that this will eventually become url.fileURLToPath(import.meta.resolve(specifier)) when we drop support for Node.js versions that lack import.meta.resolve.

I also encourage a general move toward framing Agoric APIs in terms of URLs instead of paths (as in Compartment Mapper) since they are more portable and obviate any impulse toward making this utility function.

kriskowal · 2023-08-16T18:56:04Z

packages/SwingSet/tools/paths.js

+import { resolve } from 'import-meta-resolve';
+
+/**
+ * @param {string} specifier


This function should not be. Hiding the mechanism of resolution obscures the limitations of its use. I strongly prefer inlining url.filePathToURL(resolve(specifier, import.meta.url)) since this works anywhere without qualification.

If we kept this function, we would need to express the qualifications: it can only be reliably used for absolute module specifiers like @foo/bar and not for relative module specifiers like ./foo.js within this package. The resolution of an absolute specifier is not guaranteed to be consistent between packages, differing version constraints between packages, and varying node_modules layout algorithms.

The reason for these limitations is that it uses its own import.meta.url instead of receiving the referrer specifier as a parameter.

That is to say, if present, this function should not be exported to other packages, which is antithetical to exporting it from tools.js and its usage throughout this change.

resolution of an absolute specifier is not guaranteed to be consistent between packages, differing version constraints between packages, and varying node_modules layout algorithms.

This I hadn't considered.

What if this function took parent the way import-meta-resolve does?

If this function accepted the import.meta.url from the caller, I would be converted from a “hard no” to a “soft no”, on the grounds that I’d prefer transparency at the call site. url.fileURLToPath(resolve(specifier, import.meta.url)) is very clear and not very long. Just removing the await is an improvement.

kriskowal · 2023-08-16T18:59:44Z

packages/boot/package.json

@@ -35,7 +35,7 @@
    "@endo/far": "^0.2.19",
    "@endo/promise-kit": "^0.2.57",
    "@endo/stream": "^0.3.26",
-    "import-meta-resolve": "^2.2.1"
+    "import-meta-resolve": "^3.0.0"


I’m ambivalent about upgrading import-meta-resolve to the new synchronous version, even though I’m sure it does degenerate to synchronous I/O (to inspect package.json for imports and exports clauses). The shim is not privy to the information that Node.js collected before evaluating the module in which import.meta.resolve gets expressed.

The native implementations in Node.js and the web of import.meta.resolve do not degenerate to synchronous I/O and I am sure that we can safely virtualize import.meta.resolve for Endo. Although the shim is not able to do this, it does make our code more closely resemble the eventual convergence on the (de-facto but not de-jure standard!) import.meta.resolve.

I didn't parse out the argument against upgrading it.

The downside is that the shim will perform synchronous I/O (to read package.json files). The native implementation doesn’t need to read or interpret package.json files because we can be sure Node.js already has them memoized. It had to read them in order to link the surrounding module.

The downside of inducing I/O in these cases is tenuous. It will reduce event loop availability, which is a good reason to avoid them while serving traffic or other operations. If tail latency for a server is not an issue, synchronous I/O tends overall to be faster than concurrent I/O.

I don’t fully grasp an absolute censure for I/O for OCaps. If we’re generally using this mechanism on boot-time or test-time constants, there’s not much risk of an attacker gaining the ability to indirectly tease these programs into reading more package.json files than they strictly need. But this mechanism could induce an exception.

I’d say: be careful with dynamic import, even dynamic resolution. Because it requires care, as a reviewer, I would rather see import.meta.resolve where it’s getting used rather than pkgAbsPath. It raises a flag I need to see.

kriskowal · 2023-08-16T19:03:48Z

packages/boot/test/bootstrapTests/supports.js

-  const fullPath = await importMetaResolve(specifier, import.meta.url).then(
-    u => new URL(u).pathname,
-  );
+  const fullPath = pkgAbsPath(specifier);


My preference is to inline url.fileURLToPath(importMetaResolve(specifier, import.meta.url)) here with the expectation that this will eventually become url.fileURLToPath(import.meta.resolve(specifier)) when we drop support for Node.js versions that lack import.meta.resolve.

I also encourage a general move toward framing Agoric APIs in terms of URLs instead of paths (as in Compartment Mapper) since they are more portable and obviate any impulse toward making this utility function.

kriskowal · 2023-08-16T19:05:20Z

packages/boot/test/bootstrapTests/supports.js

@@ -188,9 +188,7 @@ export const getNodeTestVaultsConfig = async (
  bundleDir = 'bundles',
  specifier = '@agoric/vm-config/decentral-itest-vaults-config.json',
 ) => {
-  const fullPath = await importMetaResolve(specifier, import.meta.url).then(
-    u => new URL(u).pathname,


I’m enthusiastic about replacing new URL(u).pathname with url.filePathToURL everywhere it’s found, in order to eventually be portable to Windows! Thank you.

kriskowal · 2023-08-16T19:07:19Z

packages/boot/test/test-boot-config.js

-  importMetaResolve(`@agoric/vm-config/${configName}`, import.meta.url).then(
-    u => new URL(u).pathname,
-  );
+  pkgAbsPath(`@agoric/vm-config/${configName}`);


The correctness of this expression relies on the incidental detail that @agoric/vm-config got installed somewhere accessible to @agoric/swingset-vat. Your pnpm experiment presumably revealed that this is not a reliable incidental detail unless @agoric/vm-config also explicitly depends upon @agoric/swingset-vat, and even then, is not guaranteed unless both of these packages depend on compatible versions.

I suspect that peerDependencies will save the day for vm-config.

kriskowal

In general, import.meta.url needs to get threaded to the call site. If there are import specifiers in a config file, import.meta.url needs to be the file URL of the config file. If the config is embedded in a JavaScript file, it should pass its own import.meta.url.

And I would rather see import.meta.url or import.meta.resolve than have them called by a utility function since there are nuances to whether they’re safe to use.

kriskowal · 2023-08-23T23:05:22Z

packages/SwingSet/tools/paths.js

+import { resolve } from 'import-meta-resolve';
+
+/**
+ * @param {string} specifier


If this function accepted the import.meta.url from the caller, I would be converted from a “hard no” to a “soft no”, on the grounds that I’d prefer transparency at the call site. url.fileURLToPath(resolve(specifier, import.meta.url)) is very clear and not very long. Just removing the await is an improvement.

kriskowal · 2023-08-23T23:14:27Z

packages/boot/package.json

@@ -35,7 +35,7 @@
    "@endo/far": "^0.2.19",
    "@endo/promise-kit": "^0.2.57",
    "@endo/stream": "^0.3.26",
-    "import-meta-resolve": "^2.2.1"
+    "import-meta-resolve": "^3.0.0"


The downside is that the shim will perform synchronous I/O (to read package.json files). The native implementation doesn’t need to read or interpret package.json files because we can be sure Node.js already has them memoized. It had to read them in order to link the surrounding module.

The downside of inducing I/O in these cases is tenuous. It will reduce event loop availability, which is a good reason to avoid them while serving traffic or other operations. If tail latency for a server is not an issue, synchronous I/O tends overall to be faster than concurrent I/O.

I don’t fully grasp an absolute censure for I/O for OCaps. If we’re generally using this mechanism on boot-time or test-time constants, there’s not much risk of an attacker gaining the ability to indirectly tease these programs into reading more package.json files than they strictly need. But this mechanism could induce an exception.

I’d say: be careful with dynamic import, even dynamic resolution. Because it requires care, as a reviewer, I would rather see import.meta.resolve where it’s getting used rather than pkgAbsPath. It raises a flag I need to see.

kriskowal · 2023-08-25T23:20:48Z

packages/SwingSet/tools/paths.js

+export const resolvePath = (specifier, base) => {
+  const href = resolve(specifier, base);
  return url.fileURLToPath(href);
 };
+
+export const makeResolvePath = base => {
+  return specifier => resolvePath(specifier, base);
+};


Alright, this is a good compromise.

@warner I would encourage us to in general migrate as much of our API surface to treating file URLs as first-class, starting with @endo/bundle-source and working our way out. @endo/compartment-mapper APIs are already framed in terms of file URLs.

kriskowal · 2023-08-25T23:23:11Z

packages/SwingSet/tools/paths.js

 * @returns {string} file system absolute path of the package resource specifier
 */
-export const pkgAbsPath = specifier => {
-  const href = resolve(specifier, import.meta.url);
+export const resolvePath = (specifier, base) => {


Maybe resolveToPath would be more clear. The arguments are both module specifiers and a path returns.

Concerns addressed.

turadg changed the title ~~WIP~~ synchronous import.meta.resolve Aug 15, 2023

turadg force-pushed the ta/meta-resolve branch from f45f6ee to c2f2dc3 Compare August 15, 2023 16:47

turadg added 13 commits August 15, 2023 16:20

WIP

346d665

more resolvePathname

e08e39a

move resolvePathname to swingset-vat

a54c366

fixup

49b01fb

build: drop extraneous import-meta-resolve

859ae99

less async

2940517

redundant awaits

f467a51

fixup

e0454b3

re-use import.meta.url for package resolution

e011f51

fixup

ba20dba

test refactor simplify

67f2cf1

simplify

a62b8a8

fixup some migration to package import

9f9f7c1

turadg force-pushed the ta/meta-resolve branch from c2f2dc3 to 9f9f7c1 Compare August 15, 2023 20:32

turadg changed the title ~~synchronous import.meta.resolve~~ simplify package path resolution Aug 15, 2023

dckc reviewed Aug 15, 2023

View reviewed changes

kriskowal previously requested changes Aug 16, 2023

View reviewed changes

kriskowal reviewed Aug 23, 2023

View reviewed changes

turadg added 3 commits August 25, 2023 13:00

thread import.meta.url

edb3e5f

try out replacing bfile

2b709ef

Merge branch 'master' into ta/meta-resolve

979f444

kriskowal reviewed Aug 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify package path resolution #8186

simplify package path resolution #8186

turadg commented Aug 14, 2023 •

edited

Loading

dckc left a comment

dckc Aug 15, 2023

dckc Aug 15, 2023

dckc Aug 15, 2023

dckc Aug 15, 2023

kriskowal Aug 16, 2023

kriskowal Aug 16, 2023

turadg Aug 16, 2023

kriskowal Aug 23, 2023

kriskowal Aug 16, 2023

turadg Aug 16, 2023

kriskowal Aug 23, 2023

kriskowal Aug 16, 2023

kriskowal Aug 16, 2023

kriskowal Aug 16, 2023

turadg Aug 16, 2023

kriskowal left a comment

kriskowal Aug 23, 2023

kriskowal Aug 23, 2023

kriskowal Aug 25, 2023

kriskowal Aug 25, 2023

		* @returns {Promise<string>} the absolute path corresponding to `specPath` if it can be
		* @returns {string} the absolute path corresponding to `specPath` if it can be

simplify package path resolution #8186

Are you sure you want to change the base?

simplify package path resolution #8186

Conversation

turadg commented Aug 14, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Aug 14, 2023 •

edited

Loading