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

Helpers design doc #94

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ This team is spun off from the [Modules team](https://github.com/nodejs/modules)

- [ ] Move loaders off thread

- [ ] Add helper/utility functions [`module`](https://nodejs.org/api/module.html) module
- [ ] Add [helper/utility functions](./doc/design/helpers.md) to the [`module`](https://nodejs.org/api/module.html) module

- [ ] Start with the functions that make up the ESM resolution algorithm as defined in the [spec](https://nodejs.org/api/esm.html#resolver-algorithm-specification). Create helper functions for each of the functions defined in that psuedocode: `esmResolve`, `packageImportsResolve`, `packageResolve`, `esmFileFormat`, `packageSelfResolve`, `readPackageJson`, `packageExportsResolve`, `lookupPackageScope`, `packageTargetResolve`, `packageImportsExportsResolve`, `patternKeyCompare`. (Not necessarily all with these exact names, but corresponding to these functions from the spec.)

- [ ] Follow up with similar helper functions that make up what happens within Node’s internal `load`. (Definitions to come.)

- [ ] Support loading source when the return value of `load` has `format: 'commonjs'`. See https://github.com/nodejs/node/issues/34753#issuecomment-735921348 and https://github.com/nodejs/loaders-test/blob/835506a638c6002c1b2d42ab7137db3e7eda53fa/coffeescript-loader/loader.js#L45-L50.
Expand All @@ -47,7 +47,7 @@ We hope that moving loaders off thread will allow us to preserve an async `resol
- [ ] Convert `resolve` from async to sync https://github.com/nodejs/node/pull/43363

- [ ] Add an async `resolve` to [`module`](https://nodejs.org/api/module.html) module

- [ ] Consider an API for async operations before resolution begins, such as `preImport` https://github.com/nodejs/loaders/pull/89

After this, we should get user feedback regarding the developer experience; for example, is too much boilerplate required? Should we have a separate `transform` hook? And so on. We should also investigate and potentially implement the [technical improvements](doc/use-cases.md#improvements) on our to-do list.
91 changes: 91 additions & 0 deletions doc/design/helpers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# Helpers

A user loader that defines a hook might need to reimplement all of Node’s original version of that hook, if the user hook can’t call `next` to get the result of the internal version. A case where this might occur is input that Node would error on, for example a `resolve` hook trying to resolve a protocol that Node doesn’t support. For such a hook, it could involve a lot of boilerplate or dependencies to reimplement all of the logic contained within Node’s internal version of that hook. We plan to create helper functions to reduce that need.

These will be added in stages, starting with helpers for the `resolve` hook that cover the various steps that Node’s internal `resolve` performs.

## Usage

The intended usage for these helpers is to eliminate boilerplate within user-defined hooks. For example:

```js
import { isBareSpecifier, packageResolve } from 'node:module';
import { readFileSync } from 'node:fs';
import { join } from 'node:path';
import { fileURLToPath, pathToFileURL } from 'node:url';


const needsTranspilation = new Set();

export function resolve(specifier, context, next) {
if (!isBareSpecifier(specifier)) {
return next(specifier, context);
}

const pathToPackage = fileURLToPath(packageResolve(specifier, context.parentURL, context.conditions));
const pathToPackageJson = join(pathToPackage, 'package.json');
const packageMetadata = JSON.parse(readFileSync(pathToPackageJson, 'utf-8'));

if (!packageMetadata.exports && packageMetadata.module) {
// If this package has a "module" field but no "exports" field,
// return the value of "module" and transpile later
// within a `load` hook
return {
url: pathToFileURL(join(pathToPackage, packageMetadata.module))
}
}
}

export async function load(url, context, next) {
if (!needsTranspilation.has(url)) {
return next(url, context);
}

// TODO: Transpile the faux-ESM in the "module" URL
// and return the transpiled, runnable ESM source
}
```

## `resolve` helpers

### `packageResolve`

Public reference to https://github.com/nodejs/node/blob/3350d9610864af3219de7dd20e3ac18b3c214c52/lib/internal/modules/esm/resolve.js#L847-L910.

Existing signature:

```js
/**
* @param {string} specifier
* @param {string | URL | undefined} base
* @param {Set<string>} conditions
* @returns {resolved: URL, format? : string}
*/
function packageResolve(specifier, base, conditions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Shouldn't conditions be part of a parameter bag? I think it's reasonable to imagine we would want to add more settings affecting the resolution down the road; making each one a separate argument would make the function difficult to use (especially when you only want to mention some of them).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the signature of the real function in https://github.com/nodejs/node/blob/3350d9610864af3219de7dd20e3ac18b3c214c52/lib/internal/modules/esm/resolve.js. We can change that, but I wasn’t thinking of that right now.

```

New signature, where many supporting functions that this function calls are passed in (and if left undefined, the default versions are used):

```js
function packageResolve(specifier, base, conditions, {
parsePackageName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are parsePackageName & the other functions below other helpers that you just didn't list for brievety?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I wanted to get feedback on this design before I wrote it out for all the other helpers. These are all functions in https://github.com/nodejs/node/blob/3350d9610864af3219de7dd20e3ac18b3c214c52/lib/internal/modules/esm/resolve.js

getPackageScopeConfig,
packageExportsResolve,
findPackageJson, // Part of current packageResolve extracted into its own function
getPackageConfig,
legacyMainResolve,
}) {
```

The middle of the function, where it walks up the disk looking for `package.json`, would be moved into a separate function `findPackageJson` that would follow a pattern similar to this, where the various file system-related functions could all be overridden (so that a virtual filesystem could be simulated, for example).

### `findPackageJson`

Extracted from `packageResolve` https://github.com/nodejs/node/blob/3350d9610864af3219de7dd20e3ac18b3c214c52/lib/internal/modules/esm/resolve.js#L873-L887 plus the `while` condition:

```js
function findPackageJson(packageName, base, isScoped, {
fileURLToPath,
tryStatSync,
})
```
2 changes: 2 additions & 0 deletions doc/design/overview.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ There are currently [three loader hooks](https://github.com/nodejs/node/tree/mas

* `globalPreload`: Defines a string of JavaScript to be injected into the application global scope.

In order to reduce boilerplate within hooks, new [helper functions](./helpers.md) will be made available.

## Chaining

Custom loaders are intended to chain to support various concerns beyond the scope of core, such as build tooling, mocking, transpilation, etc.
Expand Down