Handling of circular dependencies #1089
Comments
|
I can also reproduce this. If Rollup can't automatically fix this it would be nice if it could at least detect the potential for problem and output a warning. It looks like it should do this here but I'm not seeing such a warning emitted. |
|
@wearhere Thanks for the info about a warning (that is implemented but not printed). This would be obviously a large improvement for my case. |
|
A relatively simple (?) workaround would be to allow users to direct Rollup to load modules in a particular partial order, like "Rollup load these modules first regardless of their imports", the idea being that the user might know those imports to be lazy even if Rollup couldn't figure that out. I looked at the plugin API and didn't immediately see if/how a plugin might control this. |
|
Yes, we need to reinstate that warning (it was a casualty of a recent refactoring). Rollup adheres to the specification when it comes to module ordering – manually specifying the order isn't really a great solution (as tempting as it sounds) because you'll probably have broken behaviour if you subsequently move away from Rollup. Basically, some module (maybe your entry point) needs to import one of the modules before one of the other modules. It's often not particularly obvious which import needs to go where, sadly (see styled-components/styled-components#100 for a similar tale of woe). |
@Rich-Harris are you saying that the specification requires the broken order—that Rollup can't choose the solution @ondras outlines at top without violating the spec? |
|
@wearhere yep |
Could you please clarify how this example could be reworked? No amount of reordering the imports seems to do the trick for me. The desired output is to define |
|
To answer my last question, the only way I found to handle circular dependencies is to declare modules without any circular references, then attach functions that contain circular dependencies after-the-fact. In other words, this becomes that. This means that each class needs two modules. A private one that does not contain circular dependencies (only imports other private modules), and a public one that loads circular dependencies in the correct order. Users only import the public modules. I hope this helps others. |
|
@cowwoc It looks like your links for "this" and "that" no longer work, and I'd be really curious to see. I'm dealing with this issue in a project right now. |
|
@incompl I don't have any examples I can point to off-hand, but going by my old design notes: Here is a concrete example. Say
Makes sense? |
|
Yeah I see what you're saying. In my case, I had a group of classes that relied on each other but they were all part of one conceptual "module" so I made a new file that imports and exposes all of them. In that new file I put the imports in the right order and made sure no code accesses the classes except through the new interface. This has worked out just fine and avoids having to make a new file for each class. |
|
Circular reference warnings in Rollup have now been reinstated. Hopefully this goes some way to helping identify these issues now. |
|
Is/Can there be a way to disable circular dependency warnings? |
|
@btakita if we get feedback that these warnings are too noisy, we will definitely consider options here. |
|
+1 for some feedback that the warnings are noisy. I can only imagine it would be counterproductive to see screen-fulls (on a large project) of these warning messages when one intends to have these dependencies.
|
|
For example I'm getting a screen full of these warnings when including D3 in my project: Details |
|
//cc @Rich-Harris @lukastaegert @kellyselden see above for an example of the reinstated circular reference output. This almost gets into warning flags territory, which I guess is a standard feature for a compiler. |
|
I guess at the moment, you could supply a custom "onwarn" function to only display certain warnings. Would be nice to also be able to supply e.g. an array of warning codes to silence. To be able to do that, however, a code would need to be displayed as part of each warning so that the user knows how to silence a certain warning. |
|
Using this as a workaround. Would love a flags feature. function onwarn(warning) {
if (warning.code !== 'CIRCULAR_DEPENDENCY') {
console.error(`(!) ${warning.message}`);
}
} |
|
//cc @adrianheine |
|
I'd like in general to avoid warning about circular dependencies in |
|
Quite often, circular dependencies are the cause for some issue that's opened here or in |
|
Created #1978. |
|
To silence circular dependencies warnings for let's say moment library use: // rollup.config.js
import path from 'path'
const onwarn = warning => {
// Silence circular dependency warning for moment package
if (
warning.code === 'CIRCULAR_DEPENDENCY'
&& !warning.importer.indexOf(path.normalize('node_modules/moment/src/lib/'))
) {
return
}
console.warn(`(!) ${warning.message}`)
}
export default {
input: 'foobar.js',
onwarn,
// ...
} |
|
@incompl thanks for your suggestion - just got done removing about 10 circular dependencies. Is there any way we can add what element is being referenced circularly? This would help with debugging a TON, |
|
@mycarrysun What do you have in mind here i.e. could you sketch what output would help you here? Note that for a circular dependency warning involving |
|
More precisely, at least |
|
@lukastaegert if you could grab the imported item name? Is that possible? // utils.js
import { foo, bar } from '../../myModule'
import { baz } from '../../myOtherModule'
export function myOriginalFunction(){
foo(() => bar())
}
// circular.js
import { myCoolFunction } from './utils'
export function myCircularFunction(){
myOriginalFunction()
}
// index.js - this is the entry file
export * from './utils'
export * from './circular'Currently this would print something like: Something like this would help to know which modules are being imported circularly: I'm not sure how to handle the |
|
I see. So the exported entities listed are not necessarily exhaustive but give you examples, which culprits you should handle differently. There are still some edge cases, e.g. how to handle something like Maybe a combined output would be helpful, something like So we just list the first imported entity (or possibly I assume this would fit your needs? |
|
@lukastaegert yes that would be great!! Currently I have 4 circular dependencies and it is already starting to look crowded so I could see how the first example may be the preferred format: I like the second example if there were only 1 or 2 messages but with more than 4 or 5 it would start to be too much output. |
|
Another thing we could do to limit output would be to only every show the first circular dependency warning. I think we already do this for other types of warnings. Then you would need to tackle the warnings one-by-one, though. |
|
Yea - I think my personal preference would be to see them all at once. Or maybe limit it to up to 10 messages and then list the count of how many more messages were not displayed. |
|
Especially when rollup is configured with multiple outputs, I find this particular const silence = new Map()
function onwarn (warning, warn) {
const ignore = silence.get(warning.code)
if (ignore) {
if (ignore.has(warning.message)) return
ignore.add(warning.message)
} else {
silence.set(warning.code, new Set().add(warning.message))
}
warn(warning)
} |
|
For what it's worth, I wanted to chime in in support of this feature. I initially thought that circular dependency warnings were too noisy and contained false-positives. Fast forward a couple of months, I finally got around to investigating further and sure enough the warnings were legitimate. Every single one. This output has been extremely useful in helping me track down problems in my own code. Thank you very much for this. This feature is very useful! |
|
Agree with the previous suggestion of formatting: |
|
Figured I'd drop the consolidated function I'm using based on #1089 (comment) that also keeps the formatting performed by rollup thanks to #2271 (comment). I also included the ability to define more warning codes from a package to ignore. const onwarn = (warning, rollupWarn) => {
const ignoredWarnings = [
{
ignoredCode: 'CIRCULAR_DEPENDENCY',
ignoredPath: 'node_modules/semantic-ui-react/dist/',
},
]
// only show warning when code and path don't match
// anything in above list of ignored warnings
if (!ignoredWarnings.some(({ ignoredCode, ignoredPath }) => (
warning.code === ignoredCode &&
warning.importer.includes(path.normalize(ignoredPath))))
) {
rollupWarn(warning)
}
} |
This fixes #6. See also: rollup/rollup#1089
|
By the way, now the importer path tested may be simlified to warning.importer.startsWith('node_modules/joi/lib/') |
|
+1 |
|
Is there a simple/final workaround here to ignore circular dependency warnings for a specific library ? |
|
Can be seen here.
The problem is that the Child class cannot really inherit from the Parent, because they come out serialized in a wrong order. This is because a circular dependency on doStuff from ./lib.js exists in Parent, while doStuff depends on Child.
In this case, however, I believe that a proper solution exists: serialize lib.js first, then parent.js, then child.js (the parent-child order is necessary due to inheritance; the lib-child order can be switched as necessary, as doStuff needs the child to be present later (when executing).
The text was updated successfully, but these errors were encountered: