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

Fix ES Module loading with abolute path #3070

Merged
merged 3 commits into from Jan 21, 2022
Merged

Conversation

seydx
Copy link
Contributor

@seydx seydx commented Jan 20, 2022

♻️ Current situation

Loading ES modules with absolute paths fails on Windows because of the url scheme.

The dynamic import needs a url-sheme with file://.., but on Windows it starts with c:, which breaks the whole loading.

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)

💡 Proposed solution

The PR will transform the absolute path into a "file url"

Testing

Tested on windows and linux without any issues

Reviewer Nudging

Plugin code

@coveralls
Copy link

coveralls commented Jan 20, 2022

Pull Request Test Coverage Report for Build 1729338076

  • 1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 25.816%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/plugin.ts 1 2 50.0%
Totals Coverage Status
Change from base Build 1725864414: 0.04%
Covered Lines: 378
Relevant Lines: 1290

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1723226651

  • 1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 24.959%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/plugin.ts 1 2 50.0%
Totals Coverage Status
Change from base Build 1632316346: 0.04%
Covered Lines: 378
Relevant Lines: 1284

💛 - Coveralls

oznu
oznu previously approved these changes Jan 20, 2022
@oznu
Copy link
Member

oznu commented Jan 20, 2022

@Supereg @seydx - should this target the 1.4 beta branch?

@Supereg
Copy link
Member

Supereg commented Jan 20, 2022

@Supereg @seydx - should this target the 1.4 beta branch?

Would have included it in 1.4.1. 1.4.0 is frozen as release is imminent.

@Supereg
Copy link
Member

Supereg commented Jan 20, 2022

But if it’s super low risk I’m also open to include it last minute, if we merge it right away.

@seydx
Copy link
Contributor Author

seydx commented Jan 20, 2022

But if it’s super low risk I’m also open to include it last minute, if we merge it right away.

I do not see what speaks against it :) It would "complete" the integration of the esm PR

@longzheng
Copy link

longzheng commented Jan 21, 2022

I ran into this issue with homebridge-camera-ui on my Windows machine and @seydx figred out the solution. I can confirm the fix works (by manually patching my Homebridge with the fix)

@Supereg I would suggest this is a good fix to get out as soon as possible because otherwise plugins using ES Modules are essentially not usable on Windows.

@Supereg Supereg changed the base branch from master to beta-1.4.0 January 21, 2022 15:07
@Supereg Supereg dismissed oznu’s stale review January 21, 2022 15:07

The base branch was changed.

@Supereg Supereg merged commit 1ecfd7e into homebridge:beta-1.4.0 Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants