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

added sideffect BpmnRenderer.js … #970

Closed

Conversation

jlepinski
Copy link

@jlepinski jlepinski commented Apr 8, 2019

When extending BpmnRenderer in a build that has angular-devkit/build-optimizer optimization turned on the inherits library gets marked as pure.

I.E.
/*@__PURE__*/ inherits(BpmnRenderer, BaseRenderer);

according to the angular-devkit/build-optimizer
https://www.npmjs.com/package/@angular-devkit/build-optimizer

Some of these optimizations add /*@__PURE__*/ comments. These are used by JS optimization tools to identify pure functions that can potentially be dropped.

in the production build because inherits is marked as pure it is dropped and BpmnRenderer fails to extend BaseRenderer so errors are thrown when getConnectionPath is called. This is because the prototype for the inheritance of BaseRenderer is missing from the object.

the /*@__PURE__*/ prefix is added by the getPrefixFunctionsTransformer function in @angular-devkit/build-optimizer/src/transforms/prefix-functions.js

Since BpmnRenderer has a sideEffect of dropping inherits from build optimization when imported directly I believe it is appropriate to add this line to the sideEffect list in the package.json.

This lets the build optimization tools know not to optimize this file when imported and allows for successful extension of the BpmnRenderer.

_Which issue does this PR address? None

@nikku
Copy link
Member

nikku commented Apr 8, 2019

Could you elaborate on why that happens with AngularJS only?

We ship production builds, built with rollup, we bundle the library with webpack (all optimization options turned on) and non of these bundlers drops the inherits function because they believe it is side-effect.

Because of that I personally see this as an issue with the way Angular optimizes dependencies.

There seems to be a simple workaround for the issue, too:

import * from 'bpmn-js/...'

@jlepinski
Copy link
Author

jlepinski commented Apr 8, 2019

Just to be clear this is in Angular 7 not AngularJS.

as far as why it is tree shaken I'm not entirely sure but I'll look into it some more from their side.

It appears to me that function calls outside of the default function are treated as

"top level downleveled class declarations"
https://www.npmjs.com/package/@angular-devkit/build-optimizer

I'll play around and see if there are some other ways of modifying the module to keep inherits from being dropped.

I am a little confused about the last part of your comment.

----------------------comment-----------------------
There seems to be a simple workaround for the issue, too:

import * from 'bpmn-js/...'
---------------------- end comment------------------

Why would I want to import bpmn-js and not the renderer directly? I do see you have a ... afterward but I’m not sure what you mean by that.

Your examples of extending the renderer require the renderer directly so you can extend it.

I understand if you have an instance of the modeler already you could get the bpmnRenderer provider however I am trying to extend the renderer before the modeler is instantiated so I can add it to the additional modules on instantiation.

@nikku
Copy link
Member

nikku commented Apr 9, 2019

Why would I want to import bpmn-js and not the renderer directly? I do see you have a ... afterward but I’m not sure what you mean by that.

Sorry for the hiccup, let me clarify: We have users that reported the exact same issue when consuming the main viewer / modeler bundle. Their solution was to do

import * as BpmnViewer from 'bpmn-js/dist/bpmn-viewer.production.min.js';

instead of

import BpmnViewer from '...'

So my thought was, whether you can do import * as BpmnRenderer.

@nikku
Copy link
Member

nikku commented Apr 9, 2019

Mentioned related issue: bpmn-io/bpmn-js-example-angular#6

@jlepinski
Copy link
Author

I was able to build a work around into my own code by

importing inherits myself
if the super wasn't declared on the renderer
saving the prototype functions
running the inheritance call and
overlaying the original prototype functions on top of the updated BpmnRenderer.

I do think it probably is something that needs to be addressed with angular-cli in the long run

I read a long thread that discusses some of the ways the build optimizer breaks some third party libraries.
angular/angular-cli#11439

in the code for the build optimizer it seems the reasoning for marking it as droppable is only because it is a top level function I couldn't really find any other justification for it.

@jlepinski jlepinski closed this Apr 9, 2019
@nikku
Copy link
Member

nikku commented Apr 9, 2019

What is the exact workaround? Could you share some code? I presume other angular users will run into the same issue. It seems like the angular team neglects the fact that the build optimizer breaks third party dependencies.

@lppedd
Copy link

lppedd commented Jun 11, 2019

Still a problem here. Can't use this in production with Angular.
Related SO question I posted is here.

@jlepinski can you re-open this issue?

@lppedd
Copy link

lppedd commented Jun 11, 2019

Possible solution is a custom Builder to override Terser options. Investigating.

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

Successfully merging this pull request may close these issues.

3 participants