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

Can not import .node file in esm #40541

Closed
Brooooooklyn opened this issue Oct 21, 2021 · 26 comments
Closed

Can not import .node file in esm #40541

Brooooooklyn opened this issue Oct 21, 2021 · 26 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@Brooooooklyn
Copy link

Brooooooklyn commented Oct 21, 2021

Version

v16.12.0

Platform

x86_64-apple-darwin

Subsystem

No response

What steps will reproduce the bug?

import { fastFunction } from './native.node'

console.log(fastFunction)

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

[Function: fastFunction]

What do you see instead?

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".node" 

Additional information

No response

@Mesteery Mesteery added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 21, 2021
@iam-frankqiu
Copy link
Contributor

Maybe you should try setting the command-line options --experimental-specifier-resolution = node. The .node extension that isn't supported default has been a legacy extension. you should add the above options to support this extension.

@Brooooooklyn
Copy link
Author

Should we default support .node file?

@iam-frankqiu
Copy link
Contributor

Should we default support .node file?

I don't think it's a good idea. actually. I even don't think .mjs should support default. I like the design of simple and easy to use.

@Brooooooklyn
Copy link
Author

What's the recommend way to deliver native addon as esm package?

@Brooooooklyn
Copy link
Author

I need to wrap some annoying codes to make my package could be imported in esm:

const { func1, func2, func3, func4 } = require('./some-addon.node')

module.exports = {
  func1,
  func2,
  func3,
  func4,
}

@iam-frankqiu
Copy link
Contributor

https://nodejs.org/api/module.html#module_module_createrequire_filename
Maybe it can help you to solve the puzzle.

@Brooooooklyn
Copy link
Author

Maybe it can help you to solve the puzzle.

It doesn't help at all.
It turns codes into:

const { func1, func2, func3 } = createRequire(...)("./native.node")

export { func1, func2, func3 }

Please reopen this issue.

@iam-frankqiu iam-frankqiu reopened this Oct 22, 2021
@Brooooooklyn
Copy link
Author

@mhdawson is this issue belong to Node-API scope?

@mhdawson
Copy link
Member

@Brooooooklyn I don't think this is specific to Node-API, more related to ESM. @nodejs/modules has there been discussion around the ESM plans for loading native modules?

@bmeck
Copy link
Member

bmeck commented Oct 25, 2021

At some point in time it was ripped out and moved to

const legacyExtensionFormatMap = {
, I forget the exact reasoning but it partially dealt with not having a general MIME agreed upon / some push back since it would have people write code that didn't work in other environments / the introduction of createRequire.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Oct 25, 2021

Supporting it via import is probably something we can’t do just yet until we wait and see how the spec settles with regard to JSON and WebAssembly imports. For now I think you should be able to do:

import { createRequire } from 'module';
const require = createRequire(import.meta.url);

const nativeModule = require('./native.node');

See https://nodejs.org/api/module.html#modulecreaterequirefilename

@Brooooooklyn
Copy link
Author

Brooooooklyn commented Oct 25, 2021

As I metioned below, createRequire usage is too verbose for library authors. For example if I had 10 functions exported from native addon and I want provide esm package for these functions, I need to write these codes:

const { func1, func2, func3, func4, func5, func6, func7, func8, func9, func10 } = createRequire(...)("./native.node")

export { func1, func2, func3, func4, func5, func6, func7, func8, func9, func10 }

@MylesBorins
Copy link
Member

MylesBorins commented Oct 25, 2021

this does seem like something we could explore using import assertions.

specifically something like

import { someOutput } from './native-thing.node' assert { type: 'node-native' }

Thoughts? (ignore the type which is terrible, but this solves the ambiguity + cross platform problem we originally were concerned about with importing native extensions.

This is also one of those things we can give a super clear error message for with "copy pastable" solutions when we throw

@weswigham
Copy link

...does it? I'm not really sure the assertion changes anything - once you have a native import, the code won't be cross platform, assertion or no; all that would change would be weather the error was issued during parse or fetch (both of which are before execution anyway). I don't see why native imports would need an import assertion in node, considering you don't have to worry about an unauthorized middleman tampering with the format of the specifier. In fact, no assertion might be better, because then the native specifier can have its resolution overridden and replaced with a pure JS polyfill in a browser environment.

@GeoffreyBooth
Copy link
Member

I don’t see why native imports would need an import assertion in node, considering you don’t have to worry about an unauthorized middleman tampering with the format of the specifier.

We’re close to adding HTTPS imports, so then we’ll have many of the same concerns as the browser. And from my work in implementing support for JSON imports, I think there’s the potential for race conditions if it’s ever possible to have two imports for the same URL where one has an assertion and the other doesn’t. So in short, I think we’ll end up matching browsers on this one where assertions will always be required for all non-JavaScript formats.

@weswigham
Copy link

weswigham commented Oct 26, 2021

Again, if you have a format-locking assertion, you lose the ability to have format fallbacks in specific runtimes, so I think you'd at least need to support it without an assertion (heck that caching concern better not be a problem in practice, since you don't use assertions in conditional export/import maps). Hell, I'd argue that the assertion should only be allowed on URL imports that have that integrity security concern, that way dependencies are more reliably overridable by application authors. Having to float a patch on a dependency, rather than just provide a dependency override, to polyfill crossplatform support in it would be a bigger pain in the toolchain.

@targos
Copy link
Member

targos commented Oct 26, 2021

Maybe I'm mistaken (and I hope I am), but I don't see how we could support named exports with native modules. There wouldn't be the possibility to parse the source code to find them like we do for CommonJS.

@GeoffreyBooth
Copy link
Member

Maybe I’m mistaken (and I hope I am), but I don’t see how we could support named exports with native modules. There wouldn’t be the possibility to parse the source code to find them like we do for CommonJS.

Yes, though if the package author is willing to write an ESM wrapper, they could be created that way:

import { createRequire } from 'module';
const require = createRequire(import.meta.url);

const nativeModule = require('./native.node');
export const { foo, bar, baz } = nativeModule;

You could also cheat by writing CommonJS JavaScript that our “find named exports in CommonJS” detection algorithm will recognize:

const nativeModule = require('./native.node');

module.exports = nativeModule;
module.exports.foo = nativeModule.foo;
module.exports.bar = nativeModule.bar;
module.exports.baz = nativeModule.baz;

Neither wrapper is dynamic in that it can pick up all exported members of the native module without the wrapper needing to specify them, which I think is what @targos is getting at. I think since ESM named exports are required to be statically analyzable, they need to be specified; the only case where they aren’t is export * from, but in the case of export * from './native-module.node' we wouldn’t be able to detect what those exported members are, hence the need for a wrapper.

@Brooooooklyn
Copy link
Author

I've descided to generate the js binding file in @napi-rs/cli https://github.com/napi-rs/napi-rs/releases/tag/%40napi-rs%2Fcli%402.0.0-alpha.4.

I chose the commonjs style to make it compatible with both CommonJS and ESM: Generated js binding

So feel free to close this one.

@motla
Copy link

motla commented Dec 20, 2021

As I metioned below, createRequire usage is too verbose for library authors. For example if I had 10 functions exported from native addon and I want provide esm package for these functions, I need to write these codes:

const { func1, func2, func3, func4, func5, func6, func7, func8, func9, func10 } = createRequire(...)("./native.node")

export { func1, func2, func3, func4, func5, func6, func7, func8, func9, func10 }

@Brooooooklyn What about, in your module my-native-addon:

main.mjs
import { createRequire } from "module"
export default createRequire(import.meta.url)("./native.node")

Then in your application code

import addon from "my-native-addon"
console.log(addon.func1, addon.func2)

or

import { func1, func2 } from "my-native-addon"
console.log(func1, func2)

@Brooooooklyn
Copy link
Author

@motla I want my package to be compatible with CommonJS and esm. And I don't want to write two copies of index.js

CamJN added a commit to getargv/getargv.js that referenced this issue Jan 15, 2023
node/npm really make this shitty: nodejs/node#40541
Ashoat added a commit to CommE2E/comm that referenced this issue Feb 20, 2023
…pecifier-resolution=node

Summary:
Context in [ENG-3059](https://linear.app/comm/issue/ENG-3059/keyserver-fails-to-load-napi-compiled-module). `rust-node-addon` was broken by D6695, but `--experimental-specifier-resolution=node` is going away so we need another solution.

I got this solution from [this GitHub comment](nodejs/node#40541 (comment)).

Depends on D6783

Test Plan: Confirm I could import `rust-node-addon` and call methods on it from @jon's `jonringer/identity-deleteuserrpc` branch

Reviewers: jon, varun, max

Reviewed By: jon

Subscribers: tomek, atul, jon

Differential Revision: https://phab.comm.dev/D6784
@JonasBa
Copy link

JonasBa commented Jun 16, 2023

Something worth noting is that if you do go with the createRequire(import.meta.url), you might end up breaking bundling as the expression is no longer analyzable. The nice part about cjs is that once require is detected, most bundlers will have a way to copy the binary to your dist folder and change the import path, so everything just works.

If you do decide to go via createRequire route, you risk bailing out of that detection and end up with either build errors or runtime errors (missing bindings as they were never moved to the correct path). As a library author, I would want to see .node file imports being directly supported without us requiring to add any global helpers that risk naming collision or subtly breaking out of the build process.

@d3x0r
Copy link

d3x0r commented Aug 30, 2023

How is not supporting .node in .mjs import making this 'easy to use' ?

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".node" for (fullpathto)\sack_vfs.node

okay - fine I'll use require....

ReferenceError: require is not defined in ES module scope, you can use import instead (no I can't.)

and then early suggestions of --experimental-specifier-resolution=node is deprecated.

@GeoffreyBooth
Copy link
Member

import { createRequire } from "node:module"
const nativeModule = createRequire(import.meta.url)("./native.node")

@d3x0r
Copy link

d3x0r commented Aug 30, 2023

file1.cs

module.exports=require("./native.node" );

fileUse.mjs

import whatever from "./file1.js"

Still 2 lines - just 1 in an extra file.

@guybedford
Copy link
Contributor

Personally, I also continue to be surprised no one ever landed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

No branches or pull requests

14 participants