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

Base element's static method with nocollapse shouldn't be collapsed in subclasses. #3771

Open
dvoytenko opened this issue Feb 9, 2021 · 4 comments
Labels

Comments

@dvoytenko
Copy link
Contributor

In this scenario:

class BaseClass {
  /** @nocollapse */
  static getSomething() {}
}

class SubClass {
  /** @override */
  static getSomething() {}
}

The SubClass's getSomething static method gets collapsed and removed. However, an overriden static method like this should stay since it's kept in the superclass.

@concavelenz
Copy link
Contributor

That isn't how /** @nocollapse */ was designed to work (instead it is intended to mean "this thing right here"). Can you provide more detail of your use case?

We have been working to "soften" collapse properties but we don't have a concrete plan as of yet. However, perhaps you can get away with a weaker version of Collapse Properties (module exports only) in your project in the mean time.

@dvoytenko
Copy link
Contributor Author

The use case is a set of APIs that are used before an instance of an object is created. E.g. let's say we have a base class defined like this:

class BaseElement {

  static getPriority() {
    return 0;
  }
  
  constructor() {...}
}

And we have classes that extend this super-class:

class Element1 extends BaseElement {
  /** @override */
  static getPriority() {
    return 1;
  }
}
class Element2 extends BaseElement {
  /** @override */
  static getPriority() {
    return 2;
  }
}

Finally, we have some scheduler that interrogates the static getPriority() before creating an instance. E.g.

scheduler.createLater(Element2).then(instance => {...})

The getPriority is static and called before the instance is created. The desire is to have subclasses define their own values. However, Closure optimizer removes such static methods right now. The @nocollapse appears to work, but (a) its use is not convenient, and (b) quite possible this is not an intended use for it.

Any advice?

@brad4d
Copy link
Contributor

brad4d commented Feb 16, 2021

The best advice I can give with the current state of affairs is to explicitly put @nocollapse on every static method of a class if you intend to pass that class around to functions and have the static methods invoked as classArgument.staticMethod().
The @nocollapse annotation exists so you can tell the compiler you plan to do this. (Actually, I think it might work to just put @nocollapse on the definition of the class itself, but I don't have time just now to 100% confirm this.) You do have to do this for each class declaration. This annotation is not inherited.

For reasons related to optimization that are too complex to go into here, closure-compiler wants to treat static methods as if they were stand-alone functions that just happened to be stuck on a class. It tries really hard to rename 'SomeClass.staticMethod' to a global variable named something like 'SomeClass$staticMethod'. This will break your code if you pass class constructors around as objects and invoke static methods off of them instead of always invoking them via the original class name.

I'd love to make closure-compiler stop doing this, but if I do several important projects suddenly increase hugely in output size. I am currently working on ways to at least mitigate this problem.

@dvoytenko
Copy link
Contributor Author

If there was at all a way to make @nocollapse to be "inherited", I think that'd improve things dramatically, and perhaps not a huge lift.

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

No branches or pull requests

3 participants