search

IjzerenHein / kiwi.js

Fast TypeScript implementation of the Cassowary constraint solving algorithm 🖖
Other
249 stars 24 forks source link

there's a circular dependency #16

Open mreinstein opened 5 years ago

mreinstein commented 5 years ago

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

mreinstein commented 5 years ago

which is causing errors in bundling via rollup screen shot 2019-02-22 at 1 35 28 pm

IjzerenHein commented 5 years ago

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

mreinstein commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

see https://github.com/rollup/rollup/issues/2271 for workaround

adamhaile commented 5 years ago

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 commented 5 years ago

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.