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

moveControllerPropsToOnInit should not move explicit constructor content #13

Closed
Elberet opened this issue Feb 5, 2019 · 4 comments
Closed

Comments

@Elberet
Copy link

Elberet commented Feb 5, 2019

When the moveControllerPropsToOnInit option is enabled and a custom constructor is present at the same time, the entire content of the constructor is moved to onInit():

Input:

import Controller from "sap/ui/core/mvc/Controller"
/** @namespace example */
export default class AppController extends Controller {
  input = this.byId("input")
  constructor(id, opts) {
    logger.trace("before super")
    super(id, opts)
    logger.trace("after super")
  }
}

Output:

"use strict";

sap.ui.define(["sap/ui/core/mvc/Controller"], function (Controller) {
  /** @namespace example */
  var AppController = Controller.extend("example.AppController", {
    onInit: function onInit() {
      this.input = this.byId("input");
      logger.trace("before super");
      logger.trace("after super");

      if (typeof Controller.prototype.onInit === 'function') {
        Controller.prototype.onInit.apply(this, arguments);
      }
    }
  });
  return AppController;
});

While it's understandably necessary to move constructor content after super to where it is executed after the moved props have been initialized, statements that appear before super should remain in the constructor and run before the call to the super constructor.

An example where this is required is when trying to pass data from the HTML page through a component to the application's root view - i.e., I would like the transpiled code to look something like this:

UIComponent.extend("my.Component", {
  constructor: function(id, settings) {
    this._componentSettings = settings;
    UIComponent.apply(this, arguments);
  },
  createContent: function() {
    var settings = this._componentSettings;
    delete this._componentSettings;
    var rootView = this.getMetadata().getRootView();
    rootView.viewData = settings;
    return sap.ui.view(rootView);
  }
});

Fantastic work on the plugin, by the way. With this in an easy-to-use build pipeline I can finally force inspire my colleagues to learn modern JS! 😈

@r-murphy
Copy link
Collaborator

r-murphy commented Feb 5, 2019

@Elberet .
I'm glad you like the plugin!

I'll have a look at that. I've never had a use for constructor in a controller before so I did not handle this situation.

Something you might already be aware of is that in ES2015 classes, you should not use this before super in the constructor, as according to the spec it will be null. Might be fine in UI5 though.

Also, are you using version 6.x or 7.x (release candidate)? They'd both have the same behaviour for this, but just curious.

Ryan

@Elberet
Copy link
Author

Elberet commented Feb 5, 2019

I'm using 7.0.0-rc.2.

Also, yes, I know that this shouldn't be accessed before calling super; that's why one does not do any heavy lifting in a constructor. Unfortunately, the SAP devs frequently don't subscribe to that philosophy. 🙁 The use-case above is a workaround to avoid an outright monkey-patch, either of which only still exist today because of time and version constraints...

@r-murphy
Copy link
Collaborator

r-murphy commented Feb 6, 2019

@Elberet Please give rc3 a try. This version has a breaking change in your favour, where moveControllerPropsToOnInit no longer causes constructor code to get moved to onInit.

I added a new option moveControllerConstructorToOnInit to enable the old behaviour.

I also remembered that you could put the following jsdoc on a constructor to keep it.

/**
 * @keep
 */
constructor() {}

That wouldn't be needed for you with the new default behaviour, but if you enabled moveControllerConstructorToOnInit to get the old behaviour, you could annotate an individual constructor.

I think this new default is the better approach anyways as that @keep was a bit of a hack.

@Elberet
Copy link
Author

Elberet commented Feb 6, 2019

Seems to work; statements before super appear in the constructor before the transpiled super call, everything else gets moved to the top of onInit() just below property initializers. 👍

@Elberet Elberet closed this as completed Feb 11, 2019
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