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

Update the Babel plugin to remove empty class constructors #18057

Closed
Snuffleupagus opened this issue May 9, 2024 · 2 comments · Fixed by #18060
Closed

Update the Babel plugin to remove empty class constructors #18057

Snuffleupagus opened this issue May 9, 2024 · 2 comments · Fixed by #18060
Labels

Comments

@Snuffleupagus
Copy link
Collaborator

To avoid useless class constructors we utilize the ESLint no-useless-constructor rule, which works well in most cases.
The exception is when a constructor includes pre-processor statements that make it conditionally empty, note e.g. how the following source code:

constructor() {
if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING")) {
Object.defineProperty(this, "reset", {
value: () => (this.#id = 0),
});
}
}

is transformed into (in that case for the MOZCENTRAL build):
https://searchfox.org/mozilla-central/rev/dbd654fa899a56a6a2e92f325c4608020e80afae/toolkit/components/pdfjs/content/build/pdf.mjs#1829

Hence it seems like a good idea to extend the Babel plugin to remove class constructors that become empty during building.

/cc @nicolo-ribaudo

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented May 9, 2024

I can work on this.

Do we have cases in which we also need to remove subclass constructors like the following?

class A extends B {
  constructor(...args) { super(...args) }
}

@Snuffleupagus
Copy link
Collaborator Author

I can work on this.

Thank you!

Do we have cases in which we also need to remove subclass constructors like the following?

class A extends B {
  constructor(...args) { super(...args) }
}

That exact pattern should already be caught by no-useless-constructor, at least I think so.

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

Successfully merging a pull request may close this issue.

2 participants