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

Superclass replaced with null in production build #17711

Closed
MattiasBuelens opened this issue Sep 28, 2023 · 5 comments · Fixed by #17940
Closed

Superclass replaced with null in production build #17711

MattiasBuelens opened this issue Sep 28, 2023 · 5 comments · Fixed by #17940

Comments

@MattiasBuelens
Copy link

Bug report

What is the current behavior?

When building the following JavaScript file with Webpack in production mode, it generates invalid code that throws a TypeError at runtime:

var SuperClass = class {};

var UnusedClass = class extends SuperClass {
    constructor() {
      super();
    }
  },
  unusedVariable = new UnusedClass();

The resulting code looks like this:

new class extends(null){constructor(){super()}};

If the current behavior is a bug, please provide the steps to reproduce.

See https://github.com/MattiasBuelens/webpack-minify-issue for a full reproduction case, or open it on Stackblitz.

Run:

npm install
npm run build
npm run start

Then open the resulting app in a web browser. You should see the text "Running test...", and the following error will appear in the browser console:

Uncaught TypeError: Super constructor null of anonymous class is not a constructor
    at new document.body.innerText (main.js:1:81)
    at main.js:1:43

What is the expected behavior?

You should see the text "Test succeeded!" in the web browser. The resulting app should not throw a TypeError.

The app works fine when building in development mode, so changing mode to "development" in webpack.config.js demonstrates the expected correct behavior.

Other relevant information:
webpack version: 5.88.2
Node.js version: 18.18.0
Operating System: Windows 11 Pro 22H2 (build 22621.2283)
Additional tools: N/A

Initial investigation:
If you set optimization.minimize = false, then you get:

/******/ (() => { // webpackBootstrap
var __webpack_exports__ = {};

var SuperClass = class {};

var UnusedClass = class extends (/* unused pure expression or super */ null && (SuperClass)) {
    constructor() {
      super();
    }
  },
  unusedVariable = new UnusedClass();

/******/ })()
;

The "unused pure expression or super" comes from PureExpressionDependency.js, so the bug is probably somewhere around there.

Also interesting: if you split the variable declaration for UnusedClass and unusedVariable into two separate var statements, the bug does not show up.

var SuperClass = class {};

var UnusedClass = class extends SuperClass {
    constructor() {
        super();
    }
};
var unusedVariable = new UnusedClass();
MattiasBuelens added a commit to THEOplayer/samples-nextjs that referenced this issue Sep 28, 2023
@osmestad
Copy link

We've also hit this bug when trying to update Webpack, for us the failing code can be simplified to:

var AClass = class extends EventTarget {},
  anInstance = new AClass();

I did some regression testing:
5.89.0 fails
5.86.0 fails
5.85.0 fails
5.84.1 fails
5.84.0 works
5.83.1 works

I see 5.84.1 had some inner graph changes, I guess that might be a suspect? as the workaround of setting config.optimization.innerGraph = false; seems to work.

@alexander-akait
Copy link
Member

Yeah, it is a bug, PR welcome

@cherish2003
Copy link

@alexander-akait @MattiasBuelens it seems that the code which is given by @MattiasBuelens contains comma(,) after var UnusedClass which is improper syntax in ending of class instead semi colon (;) is used it in this case results in proper working of bundler without any bug
Screenshot:
Screenshot 2023-12-24 at 1 20 59 AM

@cherish2003
Copy link

cherish2003 commented Dec 30, 2023

@alexander-akait what are your thoughts on this

@alexander-akait
Copy link
Member

@cherish2003 I think we miss something in inner graph when we handle classes, I don't look deeply on this, but yeah, I think we don't handle variables after , and thinking we can drop the whole class, it is on my roadmap, but if you want to help - I will be glad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants