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

WIP: ESM! #127

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: ESM! #127

wants to merge 1 commit into from

Conversation

FND
Copy link
Contributor

@FND FND commented Apr 25, 2022

cf. #80

Copy link
Member

@moonglum moonglum left a comment

Choose a reason for hiding this comment

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

Really great work, thanks a lot 🙇 The remaining problem seems to be that ESM does not support "folder imports" (if I'm not mistaken, all four errors are basically the same).

So while this works:

// index.js
let { foo } = require("./example");

console.log(foo);

// example/index.js
exports.foo = 2;

This does not work:

import { foo } from "./example";

console.log(foo);

// example/index.js
export const foo = 2;

Error Message:

Error [ERR_UNSUPPORTED_DIR_IMPORT]: Directory import '/tmp/foo/example' is not supported resolving ES modules imported from /tmp/foo/index.js
Did you mean to import ../example/index.js?

So we probably need to attach an /index.js to those cases 🤔

@FND FND mentioned this pull request Jan 17, 2023
@FND
Copy link
Contributor Author

FND commented Jan 17, 2023

faucet relying on Node's module-resolution algorithm internally makes this change somewhat tricky now. Because this warrants separate investigation, I've extracted #137 so this PR can exclusively focus on ESM.

  1. implicit imports AKA bare specificers

    let { resolvePath } = require("faucet-pipeline-core/util/resolve.js");
    
    resolvePath("dummy", referenceDir);
    resolvePath("dummy/index", referenceDir);

    Both of those are expected to result in …/dummy/index.js, because that's what require.resolve does. Depending on the package, path resolution might even need to take into account the respective package.json.

    We could decide to no longer support implicit references, but the consequences are not entirely clear: End users' source code or configuration typically should not be affected, but plugins will likely need to be adjusted. For example, the plugin registry would have to use more explicit specifiers rather than just referencing "faucet-pipeline-images".

  2. lazy loading of plugins

    let { loadExtension } = require("faucet-pipeline-core/util/index.js");
    
    loadExtension("dummy", referenceDir);

    This doesn't currently resolve to …/node_modules/dummy, which is of course related to the first issue, but seems worth treating separately: While loadExtension could employ resolvePath, it doesn't have an explicit reference directory; we don't wanna rely on the current working directory, explicitly providing a reference directory every time might have cascading consequences. (As evidenced by my inability to make tests pass.)

    There's also the fact that we want to provide useful error messages when lazy loading fails, so we can't simply circumvent the issue by relying on invocations already providing the respective module reference:

    import dummy from "faucet-pipeline-dummy";
    
    export let plugins = [
        dummy,
        import("faucet-pipeline-sample")
    ];

    We might change the contract here to use thunks instead of strings:

    import dummy from "faucet-pipeline-dummy";
    
    export let plugins = [
        () => loadExtension("faucet-pipeline-js/index.js", "faucet-pipeline-js")
    ];

    That feels awkward and I'm not sure it addresses all of loadExtension's use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants