Repository navigation
Return native exports from CommonJS .node requires in ESM output - #29
Merged
Merged
Conversation
Relative and package requires only had their specifier rewritten, so CommonJS importers received the virtual module's namespace instead of the addon. Route them through processNodeFile like the other loader patterns. Fixes #28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #28.
Problem
When a CommonJS module did
require("./build/Release/addon.node")(Pattern 5) orrequire("pkg")/require(`@scope/${target}`)resolving to a.nodefile (Patterns 7/8), the plugin only rewrote the string literal. Vite then resolved the hashed path to the plugin's virtual ESM wrapper (export default nativeModule), so a CJSrequire()got the module namespace, not the addon:syntheticNamedExports, so named properties are missing entirely:syntheticNamedExports, but the value carried an extradefaultkey, diverging from Node semantics.The node-gyp-build, bindings, and swift-node patterns already replace the whole call via
processNodeFile, which appends.defaultfor ESM output. Patterns 5, 7, and 8 were the outliers.Fix
processNodeFilewhen the callee isrequireor a trackedcreateRequirevariable. Other callees keep the literal-only rewrite, so e.g.someFn("./addon.node")isn't turned into an addon load.processNodeFile.Resulting output in ESM builds:
Behavior change (intentional)
On Vite ≤7, CJS code previously received a Rollup interop wrapper instead of the addon itself. That was wrong compared to Node, and this PR drops it:
Object.keys(addon)['default', ...]addon === realNativeObjectfalsetrueaddon.defaultundefined(same as Node)Only code that explicitly read
require("./x.node").default(which isundefinedin plain Node) would notice. ESM importers of CJS modules and CJS output builds are unchanged.Tests
test/vite-static-import-compat.test.ts, run against both Vite 7 and 8, covering relative-path and package requires. It builds a CJS lib that doesclass X extends addon.Base, imports the built bundle, and asserts the exports have nodefaultkey. It failed before the fix on both versions (Vite 8 with the exact error from the issue).test/napi-rs.test.tsthat assumed the barerequire("./hash.node")shape; ESM builds now emitrequire("./hash.node?native-require=…").default.pkcs11js, Vite 8.3.0): the build now runs and prints the addon exports.pnpm test(209 passing) andpnpm lintpass.