Skip to content

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 added 2 commits January 17, 2023 10:31
this is required in preparation of switching to ESM, where synchronous
`require` will have to be replaced with dynamic `import`

note that this constitutes a breaking change for developers
@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.

3 participants