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

Crash inside escodegen when minifying JavaScript class statement. #1303

Open
kevincox opened this issue Apr 4, 2024 · 10 comments
Open

Crash inside escodegen when minifying JavaScript class statement. #1303

kevincox opened this issue Apr 4, 2024 · 10 comments

Comments

@kevincox
Copy link
Contributor

kevincox commented Apr 4, 2024

// www/foo.js

export class Foo {
	foo = true;
}
// build.js
(async ()=>{
	let AssetGraph = require("assetgraph");

	let assetGraph = new AssetGraph({ root: "www/" });

	await assetGraph.loadAssets("*.js"); // Load all Html assets in the root dir

	for (let js of assetGraph.findAssets({type: "JavaScript"})) {
		js.minify();
	}

	await assetGraph.writeAssetsToDisc({ protocol: "file:" }, "out/");
})()

Result:

TypeError: this[type] is not a function
    at CodeGenerator.generateExpression (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:2516:28)
    at /home/kevincox/Downloads/node_modules/escodegen/escodegen.js:1133:38
    at withIndent (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:581:9)
    at CodeGenerator.ClassBody (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:1128:13)
    at CodeGenerator.generateStatement (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:2529:33)
    at CodeGenerator.ClassDeclaration (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:1159:30)
    at CodeGenerator.generateStatement (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:2529:33)
    at CodeGenerator.ExportNamedDeclaration (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:1237:42)
    at CodeGenerator.generateStatement (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:2529:33)
    at CodeGenerator.Program (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:1718:43)
    at CodeGenerator.generateStatement (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:2529:33)
    at generateInternal (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:2550:28)
    at Object.generate (/home/kevincox/Downloads/node_modules/escodegen/escodegen.js:2618:18)
    at get text [as text] (/home/kevincox/Downloads/node_modules/assetgraph/lib/assets/JavaScript.js:81:21)
    at get rawSrc [as rawSrc] (/home/kevincox/Downloads/node_modules/assetgraph/lib/assets/Text.js:82:30)
    at Promise.map.concurrency (/home/kevincox/Downloads/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js:60:19)
    at tryCatcher (/home/kevincox/Downloads/node_modules/bluebird/js/release/util.js:16:23)
    at MappingPromiseArray._promiseFulfilled (/home/kevincox/Downloads/node_modules/bluebird/js/release/map.js:68:38)
    at PromiseArray._iterate (/home/kevincox/Downloads/node_modules/bluebird/js/release/promise_array.js:115:31)
    at MappingPromiseArray.init (/home/kevincox/Downloads/node_modules/bluebird/js/release/promise_array.js:79:10)
    at MappingPromiseArray._asyncInit (/home/kevincox/Downloads/node_modules/bluebird/js/release/map.js:37:10)
    at _drainQueueStep (/home/kevincox/Downloads/node_modules/bluebird/js/release/async.js:97:12)

Versions

assetgraph: 7.12.0
node: 20.11.1

package-lock.json

@kevincox
Copy link
Contributor Author

kevincox commented Apr 4, 2024

Inserting a print statement into escodegen I see that the unknown type is PropertyDefinition.

@kevincox
Copy link
Contributor Author

kevincox commented Apr 4, 2024

% rg -w PropertyDefinition node_modules
node_modules/acorn/dist/acorn.d.mts
375:  body: Array<MethodDefinition | PropertyDefinition | StaticBlock>
479:export interface PropertyDefinition extends Node {
480:  type: "PropertyDefinition"
565:export type AnyNode = Statement | Expression | Declaration | ModuleDeclaration | Literal | Program | SwitchCase | CatchClause | Property | Super | SpreadElement | TemplateElement | AssignmentProperty | ObjectPattern | ArrayPattern | RestElement | AssignmentPattern | ClassBody | MethodDefinition | MetaProperty | ImportSpecifier | ImportDefaultSpecifier | ImportNamespaceSpecifier | ExportSpecifier | AnonymousFunctionDeclaration | AnonymousClassDeclaration | PropertyDefinition | PrivateIdentifier | StaticBlock

node_modules/acorn/dist/acorn.d.ts
375:  body: Array<MethodDefinition | PropertyDefinition | StaticBlock>
479:export interface PropertyDefinition extends Node {
480:  type: "PropertyDefinition"
565:export type AnyNode = Statement | Expression | Declaration | ModuleDeclaration | Literal | Program | SwitchCase | CatchClause | Property | Super | SpreadElement | TemplateElement | AssignmentProperty | ObjectPattern | ArrayPattern | RestElement | AssignmentPattern | ClassBody | MethodDefinition | MetaProperty | ImportSpecifier | ImportDefaultSpecifier | ImportNamespaceSpecifier | ExportSpecifier | AnonymousFunctionDeclaration | AnonymousClassDeclaration | PropertyDefinition | PrivateIdentifier | StaticBlock

node_modules/acorn/dist/acorn.js
1565:    return this.finishNode(field, "PropertyDefinition")

node_modules/acorn/dist/acorn.mjs
1559:  return this.finishNode(field, "PropertyDefinition")

node_modules/terser/lib/mozilla-ast.js
434:        PropertyDefinition: function(M) {
448:                    throw new Error("Non-Identifier key in PropertyDefinition");
720:                        : p.type == "PropertyDefinition" || p.type === "FieldDefinition" ? (p.key === M && p.computed || p.value === M ? AST_SymbolRef : AST_SymbolClassProperty)
1606:                type: "PropertyDefinition",
1618:                type: "PropertyDefinition",

node_modules/terser/dist/bundle.min.js
7410:        PropertyDefinition: function(M) {
7424:                    throw new Error("Non-Identifier key in PropertyDefinition");
7696:                        : p.type == "PropertyDefinition" || p.type === "FieldDefinition" ? (p.key === M && p.computed || p.value === M ? AST_SymbolRef : AST_SymbolClassProperty)
8582:                type: "PropertyDefinition",
8594:                type: "PropertyDefinition",

node_modules/uglify-js/lib/mozilla-ast.js
176:        PropertyDefinition: function(M) {
741:            type: "PropertyDefinition",

node_modules/estraverse/estraverse.js
130:        PropertyDefinition: 'PropertyDefinition',
208:        PropertyDefinition: ['key', 'value'],

@kevincox
Copy link
Contributor Author

kevincox commented Apr 4, 2024

It seems like the latest escodegen doesn't support this.

I see estools/escodegen#438. However it creates a handler for FieldDefinition not PropertyDefinition.

I see estools/escodegen#443 which creates PropertyDefinition however talks about state class fields. So unless the field is being promoted to static by Terser I'm not sure which is right without more research.

@kevincox
Copy link
Contributor Author

kevincox commented Apr 4, 2024

This same issue appears to exist on master assetgraph/assetgraph#40f98a051c4e5ba4ff0b892af0641688a884c03f.

@papandreou
Copy link
Member

@kevincox, would you be up for contributing a fix to escodegen?

@kevincox
Copy link
Contributor Author

kevincox commented Apr 6, 2024

Unlikely to happen soon. But maybe at some point.

@papandreou
Copy link
Member

In any case, that's where it has to be implemented.

A good starting point would be to look at the estree spec for PropertyDefinition and working up a test case for escodegen.

@papandreou
Copy link
Member

I guess we could also try to replace escodegen with another module, such as astring. That's just a bit more complicated, as we've exposed some of escodegen's flags in different places.

@kevincox
Copy link
Contributor Author

kevincox commented Apr 7, 2024

I ended up implementing it: estools/escodegen#465

@papandreou
Copy link
Member

Excellent! 💪

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

No branches or pull requests

2 participants