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

Add support for ESM modules and async plugin initializers. #2915

Merged
merged 3 commits into from Oct 25, 2021
Merged

Add support for ESM modules and async plugin initializers. #2915

merged 3 commits into from Oct 25, 2021

Conversation

ShogunPanda
Copy link
Contributor

@ShogunPanda ShogunPanda commented Apr 28, 2021

Add support for ESM modules and async plugin initializers

Add support for ESM modules and async plugin initializers.

♻️ Current situation

Today, if you try to load a package which type=module in package.json it throws an error as it tries to use require() on a ESM module.

💡 Proposed solution

This PR introduces detection of ESM module and if detect it uses await import().

Implications

By using await import, by definition the entire plugin loading phase has become asynchronous. All affected code places have been refactored to be async function and to await for plugin loading and initialization.

Almost for free, HomeBridge plugins can now export a async function as default and this will be awaited. This means that now plugin can do async initialization.

Reviewer Nudging

Plugin and PluginManager code.

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.
I'm not sure about the support for async functions exported by a homebridge plugin, everything else looks good 👍
Have you tested this with a ESM module?

src/pluginManager.ts Outdated Show resolved Hide resolved
src/pluginManager.ts Show resolved Hide resolved
@Supereg Supereg requested a review from oznu April 28, 2021 12:07
@ShogunPanda
Copy link
Contributor Author

Yup, I tested with a custom module and it works awesomely. The only thing I haven't tested is child bridges.

@ShogunPanda
Copy link
Contributor Author

Hello! Any chance for this to be merged?

@Fish2
Copy link

Fish2 commented Sep 13, 2021

any news of pulling this in ?

Copy link

@dgreif dgreif left a comment

Choose a reason for hiding this comment

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

Hey @ShogunPanda, this looks great! Sorry it's been sitting for so long! If you don't mind addressing at least the import comment, then I'd be happy to spin this up locally and verify it's working against an ESM version of one of my plugins.

src/plugin.ts Show resolved Hide resolved
src/pluginManager.ts Show resolved Hide resolved
@ShogunPanda
Copy link
Contributor Author

@dgreif As all comments are resolved, I just rebased my PR against the latest master, so this should be good to go.

@oznu oznu changed the base branch from master to beta October 25, 2021 10:46
Copy link
Member

@oznu oznu left a comment

Choose a reason for hiding this comment

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

Approved for beta branch.

Beta v1.3.6-beta.1

@oznu oznu merged commit f26307a into homebridge:beta Oct 25, 2021
@oznu
Copy link
Member

oznu commented Oct 25, 2021

@bwp91
Copy link
Contributor

bwp91 commented Oct 25, 2021

On line 60 in plugin.ts, I believe this needs to be changed from

    this.main = packageJSON.main || "./index.js";

to something like

    this.main = packageJSON.main || packageJSON.exports || "./index.js";

As it seems an ESM package.json example "exports": "./lib/index.js", the path is not being respected.

Edit, maybe not. I didn't realise the "main" field was still recommended for a ES module too.

@ShogunPanda
Copy link
Contributor Author

Yup, I think so. Probably I would have been more restrictive to something like:

this.main = packageJSON.main || (typeof packageJSON.exports === 'string' ? packageJSON.exports : null) || "./index.js";

But for now seems to be fine. Let's see if this becomes an issue.

@Supereg
Copy link
Member

Supereg commented Nov 19, 2021

We have most likely an Issue related to this PR. Could one please investigate: #3013 (comment)?

EDIT: I think the issue is the migration of the for loop in the pluginManager.ts. Inside the plugin init error handling was a return (which worked previously) but now skips the whole plugin init iteration. Will prepare a fix, once the author confirms by suspicions.

@seydx
Copy link
Contributor

seydx commented Jan 19, 2022

Hey guys, does anyone else have the following problem on Windows?

Error: Only file and data URLs are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'
    at new NodeError (node:internal/errors:371:5)
    at defaultResolve (node:internal/modules/esm/resolve:1016:11)
    at ESMLoader.resolve (node:internal/modules/esm/loader:422:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:222:40)
    at ESMLoader.import (node:internal/modules/esm/loader:276:22)
    at importModuleDynamically (node:internal/modules/cjs/loader:1041:29)
    at importModuleDynamicallyWrapper (node:internal/vm/module:437:21)
    at importModuleDynamically (node:vm:381:46)
    at importModuleDynamicallyCallback (node:internal/process/esm_loader:35:14)
    at eval (eval at <anonymous> (C:\Users\Long\AppData\Roaming\npm\node_modules\homebridge\src\plugin.ts:24:24), <anonymous>:3:1)

const _importDynamic = new Function("modulePath", "return import(modulePath)");

@ShogunPanda
Copy link
Contributor Author

Most likely it will happen to everybody on Windows.
I don't have a Windows PC and only developed on MacOS or Linux.

@seydx
Copy link
Contributor

seydx commented Jan 20, 2022

Most likely it will happen to everybody on Windows.
I don't have a Windows PC and only developed on MacOS or Linux.

Will post a PR later with a fix

edit: see #3070

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.

None yet

7 participants