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

CommandInterceptor.js has side-effects, but is not included in sideEffects property in package.json #353

Closed
Yogu opened this issue Jun 4, 2019 · 8 comments
Labels
pr welcome We rely on a community contribution to improve this. toolkit

Comments

@Yogu
Copy link

Yogu commented Jun 4, 2019

We use dmn.js in an angular app. In development mode, it works fine, but in a prod build, we get the following error:

ERROR TypeError: i.canExecute is not a function
    at main.5ccc82491d1ace385dd5.js:formatted:130117
    at Array.forEach (<anonymous>)
    at oD.iD.addRule (main.5ccc82491d1ace385dd5.js:formatted:130116)
    at oD.init (main.5ccc82491d1ace385dd5.js:formatted:130128)
    at oD.iD (main.5ccc82491d1ace385dd5.js:formatted:130080)
    at new oD (main.5ccc82491d1ace385dd5.js:formatted:130083)
    at Array.c (main.5ccc82491d1ace385dd5.js:formatted:123834)
    at e (main.5ccc82491d1ace385dd5.js:formatted:123812)
    at main.5ccc82491d1ace385dd5.js:formatted:123828
    at Array.map (<anonymous>)

This is the compressed code around the error location:

        iD.prototype.addRule = function(e, t, n) {
            var i = this;
            "string" == typeof e && (e = [e]),
            e.forEach(function(e) {
                i.canExecute(e, t, function(e, t, i) {
                    return n(e)
                }, !0)
            })
        }

The problem is that the canExecute method is missing in the CommandInterceptor prototype. Enabling source maps shows that the hooks actually are not created - the whole part of CommandInterceptor starting from the hooks array delcaration is optimized away, specifically, this:

var hooks = [
  'canExecute',
  'preExecute',
  'preExecuted',
  'execute',
  'executed',
  'postExecute',
  'postExecuted',
  'revert',
  'reverted'
];

/*
 * Install hook shortcuts
 *
 * This will generate the CommandInterceptor#(preExecute|...|reverted) methods
 * which will in term forward to CommandInterceptor#on.
 */
forEach(hooks, function(hook) {

  /**
   * {canExecute|preExecute|preExecuted|execute|executed|postExecute|postExecuted|revert|reverted}
   *
   * A named hook for plugging into the command execution
   *
   * @param {String|Array<String>} [events] list of commands to register on
   * @param {Number} [priority] the priority on which to hook into the execution
   * @param {Function} handlerFn interceptor to be invoked with (event)
   * @param {Boolean} [unwrap=false] if true, unwrap the event and pass (context, command, event) to the
   *                          listener instead
   * @param {Object} [that] Pass context (`this`) to the handler function
   */
  CommandInterceptor.prototype[hook] = function(events, priority, handlerFn, unwrap, that) {

    if (isFunction(events) || isNumber(events)) {
      that = unwrap;
      unwrap = handlerFn;
      handlerFn = priority;
      priority = events;
      events = null;
    }

    this.on(events, hook, priority, handlerFn, unwrap, that);
  };
});

I don't know too much about tree shaking, but I think the problem is that diagram.js is declared side-effect free (only css files are listed in sideEffects in package.json), but this file contains imperative code that is not clearly linked to one of the exports.

I didn't find an explicit list of what is considered a side-effect. Maybe it would already help to use a standard for-of and not call forEach of min-dash. But I think the safest way would be to assign the hooks explicitly to the prototype (you could still use a helper function to generate the hook implementation).

Steps to Reproduce

  1. Create an angular app and add dmn-js as a dependency
  2. Import it with import Modeler from 'dmn-js/lib/Modeler'
  3. Instanciate it with new Modeler(/* ... */)
  4. Build it with ng build --prod (which enables tree shaking and other optimizations)

I don't think it's relevant how exactly we use dmn-js, but if needed I can try to create a sample app that reproduces the problem.

Environment

  • Browser: Chrome 74, Firefox 60 ESR
  • OS: Windows 10
  • diagram-js version: 3.3.0
@nikku
Copy link
Member

nikku commented Jun 4, 2019

This is due to angular applying very aggressive optimization. This caused issues in other parts of our toolbelt already, cf. angular example issue, this forum post.

No other cli tool or bundler shows this behavior.

We are open to pull requests that improve the command interceptor implementation so that it is side-effect free from the Angular CLI point of view. Another option is to pursue the angular guys to fix their tool to not optimizing that particular pattern.

@nikku nikku added the pr-needed label Jun 4, 2019
@Yogu
Copy link
Author

Yogu commented Jun 4, 2019

Thanks for the reply. I already found a lot of issues in the meantime, this one here will probably only be the first we would encounter. I'm currently trying the workaround to import the prod builds of the library and see if this fixes it.

@nikku
Copy link
Member

nikku commented Jun 4, 2019

Using the prod builds is the known workaround for such optimization issues.

You can even bundle your own version of the editor, if you perform heavy customization.

@nikku
Copy link
Member

nikku commented Jun 4, 2019

Closing this, as we do not intend to fix such issues. We are open for pull requests that improve the situation for angular users though.

@nikku nikku closed this as completed Jun 4, 2019
@nikku nikku added the toolkit label Jun 4, 2019
@nikku
Copy link
Member

nikku commented Jun 4, 2019

Related angular-cli issue for breaking external libraries: angular/angular-cli#11439

@Yogu
Copy link
Author

Yogu commented Jun 4, 2019 via email

@lppedd
Copy link

lppedd commented Jun 12, 2019

@nikku the main problem is the inherits function. I have no idea honestly what could be a side effect free alternative. You?

@nikku
Copy link
Member

nikku commented Jun 12, 2019

Please understand that the main problem is the angular-cli. Please provide your feedback to the Angular developers so that they can fix their tool chain.

@nikku nikku added the pr welcome We rely on a community contribution to improve this. label Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr welcome We rely on a community contribution to improve this. toolkit
Projects
None yet
Development

No branches or pull requests

3 participants