closes #652: class methods from precompiled .js drop on instance (v0.5.795) - #664
Merged
Merged
Conversation
…5.795)
Pre-fix typeof p.query was undefined for any class imported from a .js
file in node_modules: cjs_wrap left class declarations INSIDE the
synthetic IIFE, so HIR saw them as exported: false and the consumer's
named import resolved to a runtime _cjs.X property access (not the
class). new ClassRef(...) allocated an empty ObjectHeader with no
ctor + no methods; p.query / p.url / Object.keys(p) all empty.
Fix: new extract_top_level_class_decls helper does brace-balanced
scan of CJS source, hoists top-level class declarations OUT of the
IIFE to outer module scope, emits direct `export { X }` re-exports.
The IIFE body still has `exports.X = X` referencing the hoisted
class via closure, so default-export `_cjs.X` shape keeps working.
Detection is conservative — only column-0 + optional indent class
keyword — so nested classes stay inside the IIFE.
Validation: new regression test test-files/issue_652/ with extends
+ super() + async methods + multiple classes. 7 sanity tests
byte-identical to Node.
Out of scope: classes whose body references module/exports/require
directly — uncommon for precompiled npm output, filed separately.
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.
Summary
Closes #652 — class methods from precompiled
.jsfiles innode_modulesdrop on instance.typeof p.querywasundefined, fields silently missing, blocking any precompiled npm-published package that exposes a class with instance methods (@perryts/mysql'sPool/Connection, several others).Root cause:
cjs_wrap.rsleft everyclass X { ... }declaration INSIDE the synthetic IIFE that re-shapesmodule.exports. Perry's HIR saw the class asexported: false(it lived in a closure body, not at module scope), and the consumer's named import resolved to_cjs.X— a runtime property access, not the class declaration.new ClassRef(...)then allocated an empty*ObjectHeaderwith no constructor + no methods.Fix: new
extract_top_level_class_decls(source)helper does a brace-balanced scan (handles strings / template literals / line + block comments) of the CJS source and hoists top-levelclass X { ... }declarations OUT of the IIFE to the outer module scope. The IIFE body still has the originalexports.X = Xreference, but it now resolves via closure. Hoisted class names are emitted asexport { X };(direct re-export) so the consumer's named import resolves to the actual class. Detection is conservative — onlyclasskeyword at column-0 + optional indent — so nested classes inside functions / blocks stay inside the IIFE.Test fixture
New
test-files/issue_652/exercises the full pattern:node_modules/minilib/index.jswithclass Base { greet() {} }+class Pool extends Base { async query() {} }+exports.{Base,Pool} = ...main.tsimports both, instantiates them, calls instance methods + super() + async methodsOut of scope
Classes whose body references
module/exports/requiredirectly (class X { static foo = require('./y') }) — uncommon for precompiled npm output; the current implementation hoists the class block before the IIFE runs, so anyrequire()inside the class body wouldn't work. Filed as a separate followup if it surfaces.Test plan
new Pool({url}).query("SELECT 1")returns{rows:[{sql,ok:true}]}