there's a circular dependency

[mreinstein]

expression.ts -> variable.ts -> expression.ts

[mreinstein]

which is causing errors in bundling via rollup

[IjzerenHein]

I know, it's been in there for a long time. Is it causing problems for you?

[mreinstein]

actually it appears that rollup is able to deal with this, it isn't causing breakage. It would be a nice design thing to fix, but not nearly as critical as I initially thought.

[IjzerenHein]

Agreed. I Checked the code but couldn't find a quick solution. I guess it would require merging some files to resolve this issue, but I doubt that is what we want.

[leeoniya]

see rollup/rollup#2271 for workaround

[adamhaile]

My understanding is that circular dependencies are a warning in rollup (yellow), not an error (red), because in a lot of cases they're fine. For instance, they're touted by the rollup docs as a feature that es6 modules have over CommonJS. Rollup can't (yet) distinguish the ok cases from the not ok, so it currently always warns.

The usage here, between expression.ts and variable.ts, looks fine to me. I propose closing?

[mreinstein]

I Checked the code but couldn't find a quick solution.

@IjzerenHein we could eliminate the circular dependency by moving the Variable.plus, Variable.minus, Variable.multiply, and Variable.divide functions into Expression. Those are the only references to expressions from with the variable module. And since they are creating expressions it might be reasonable to put them there anyway. perhaps the function prototype changes from

minus( value: number|Variable|Expression ): Expression

to

minus( value: number|Variable|Expression,  value2: number|Variable|Expression ): Expression`)

I realize this might be controversial as it would be a major api semver bump for the sake of fixing a non-critical bundling error, but it wouldn't be hard to do this.