reduce Circular dependency noise

[leeoniya]

Hi guys,

I'm seeing a lot of Circular dependency warnings and think the situation can be improved without too much difficulty.

Simply put, if the imported items are only functions (rather than statements/expressions), then bail on any additional circular dependency checks, since function declaration order does not matter.

This could be a more elegant solution than chopping them off wholesale as #1978 would do, and that issue is stalled anyhow.

A simple example that's common in processing tree structures:

index.js

export { processNode } from './processNode';

processNode.js

import { processChildren } from './processChildren';

export function processNode(node) {
  node.foo = 'bar';
  processChildren(node);
}

processChildren.js

import { processNode } from './processNode';

export function processChildren(node) {
  node.children.forEach(processNode);
}

rollup.config.js

export default {
  input: 'index.js',
  output: {
    name: 'processNode',
    file: 'bundle.js',
    format: 'iife'
  }
};

bundle.js

var processNode = (function (exports) {
	'use strict';

	function processChildren(node) {
		node.children.forEach(processNode);
	}

	function processNode(node) {
		node.foo = 'bar';
		processChildren(node);
	}

	exports.processNode = processNode;

	return exports;

}({}));

thanks!

[lukastaegert]

An interesting idea.

But once the functions are called, order would matter again, wouldn't it, at least if the calls are in the critical path? Just trying to get a clear picture of how such an analysis could work.

I cannot make any promise that this will be resolved any time soon. But @guybedford has always wanted to have some information about what is necessary for the critical path for various reasons anyway. Assuming we have some way to get this information, it would be possible to restrict circular dependency warnings to code in the critical path. This would solve the issue in your example as all of the code is just called from an exported function which means it is not in the critical path for this bundle.

[guybedford]

@leeoniya this is an interesting suggestion, I just wonder if there might still be ordering issues with function declarations, when dealing with function declarations that access local bindings that might still be in the TDZ.

[leeoniya]

when dealing with function declarations that access local bindings that might still be in the TDZ

not sure i follow. as long as any invocation is done afterwards, the following is still fine. the location of the declaration doesnt matter, right?

function foo() {
  console.log(bar);
}

let bar = 1;

foo();

also fine:

let bar = 1;

foo();

function foo() {
  console.log(bar);
}

[guybedford]

@leeoniya cases like:

a.js

import { fn as bfn } from './b.js';

export function fn () {
  console.log('afn');
}

bfn();

b.js

import { fn as afn } from './a.js';
let x = 5;
export function fn () {
  console.log(x);
}
afn();

where <script type=module src=a.js> will throw that x is in TDZ.

[guybedford]

(updated the example to be a bit more practical)

[leeoniya]

is it not possible to simply hoist any locals in a post pass?

[guybedford]

@leeoniya that would be a spec violation. It's important to have spec behaviours exactly for predictability of implementations.

[leeoniya]

i suppose you can still emit the Circular warning if both imports access locals, but if it's only one then it seems fairly deterministic?

[guybedford]

I think the exact exception would be skipping a circular warning if only functions are being imported by the circular module executing first AND all of those functions have variables that are not in TDZ at the time of execution of the importer.

As @lukastaegert says, if we have execution order analysis / TDZ analysis then we could possibly use this to determine the above and do that.

[leeoniya]

cool, i leave this issue in your capable hands :P

[leeoniya]

as an initial step, is it already possible to avoid circular warnings for pure functions that use no locals at all?

[lukastaegert]

At the moment, circular dependency handling only looks at the modules and their imports/exports as a whole so implementing something like this would not be easy.

As a simple first step without TDZ analysis (which might therefore still leave some unwanted warnings but would solve your example), we could run the normal tree-shaking algorithm first without including any external exports.

This would mean that only code that can be reached via the critical path would be included (But also some asynchronous code as we currently have no way of knowing if a global or external function like Promise.resolve().then() or setTimeout() is calling its arguments synchronously. But this could be added in the future.). Then we use this information to print cyclic dependency warnings only for the modules included at that point. Then we manually include the external exports (if there are any, otherwise we are finished at that point) and continue running the tree-shaking algorithm as before.

All in all, this would probably be somewhat slower than just including the exports right away but it would make these warnings much more relevant. Most libraries tend to execute only very little code during startup. Also, this simple heuristic could enable other optimisations later.

[guybedford]

Warning for circular bindings touched only on the critical execution path definitely sounds like an improvement.

But I'm not sure I follow here how running tree-shaking without external exports would achieve this? Does this mean we already have the pieces in place for critical path analysis 😮

[lukastaegert]

I think we have most of the pieces, yes. At least we could have a good heuristic that should work well for libraries.

For webapps, which do not have external exports but instead rely on globals to call asynchronous code, we would need to add code to track Promises and timeouts to know where the synchronous path ends. But as all of them are globals, this should be doable.

[curran]

Related issue D3: Circular dependencies inside d3 selection.

As the ES6 Spec supports cyclic dependencies, I'm not sure why Rollup would show these warnings by default. Avoiding cyclic dependencies in ES6 modules seems to be a "best practice", more of a stylistic issue, so these warnings feel more ESLint-ish.

This may be an outlandish idea, but what if these cyclic dependency warnings overall were made opt-in in Rollup, rather than opt-out?

[wycats]

I'm getting a ton of these in Glimmer, and while I appreciate the sentiment, many circular dependencies are not just acceptable, but supported by design!

What if circular dependency warnings were limited to cases where the cycles affected top-level expressions (most commonly the right-hand-side of extends clauses), which actually do indicate an order sensitivity that could bite the user in the future?

(edit: I see this was proposed above. Until the critical-path heuristic is applied, can I suggest making the warning opt-in as @curran suggests?)

[TrySound]

@wycats I ignore circular dependency I can't deal with

  onwarn(warning) {
    const ignoredCircular = [
      'react-virtualized',
      'd3-interpolate',
      'libphonenumber-js',
      'viewport-mercator-project',
    ];
    if (
      warning.code === 'CIRCULAR_DEPENDENCY' &&
      ignoredCircular.some(d => warning.importer.includes(d))
    ) {
      return;
    }
    throw Error(warning.message);
  },

[leeoniya]

@lukastaegert

is there any progress on the horizon for this?

it's really silly that the "solution" to this self-inflicted problem is to ignore these warnings. why not just make them opt-in and forget the whole thing? what was the reasoning behind making them opt-out in the first place? asking for a friend ;)

[lukastaegert]

Nothing in progress here unfortunately. Personally I still think that circular dependencies are more often unintended and lead to hard to understand bugs especially for less experienced people. This is why I prefer the opt-out approach, otherwise I expect more issues to float towards Rollup where people expect me to explain to them the circular dependency resolution logic which I honestly do not have the resources to handle. The shortest approach IMO that retains all other logic is

onwarn(warning, rollupWarn) {
  if (warning.code !== 'CIRCULAR_DEPENDENCY') {
    rollupWarn(warning);
  }
}

Should put this into the docs I guess.

[shellscape]

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

[leeoniya]

The issue will still be here, just in a closed state.

heh, cute.

[shellscape]

@leeoniya hey there. It wasn't mean to be cute, kitchy, witty, or sassy. There are some users that may legitimately confuse a closed state with deletion, etc. The form-message was curated to be warm. Sorry if it didn't come across as such. Rollup has a mountain of issues and we're starting a push to get the ones that still need love, said love.

[leeoniya]

Rollup has a mountain of issues and we're starting a push to get the ones that still need love, said love.

a mountain of issues is poor justification for closing of issues. issue tags exist, and can easily be used to deprioritize and categorize as needed. closing them out is the equivalent to pretending signalling they no longer exist on any future radar (end of discussion). i suspect that zero maintainers ever go back to closed issues and continue working on them.

to be clear, i think this specific issue is ok to close regardless, since the workaround is trivial. however, there were other issues closed that were obviously not resolved; so much so that i thought @shellscape was a stale auto-close bot :)

anyways, i can sympathize with maintaining open source projects for free and have much respect for you guys for pushing rollup forward - a huge asset to the webdev community.

[lukastaegert]

I agree that closed issues have little chance to be looked at again but as it stands with the low number of outside contributions, we are fighting an uphill battle where the number of new issues always outweighs the number of resolved issues.
Thus if an issue was not too critical, its chance of being looked at was equally low, and important issues get overlooked.

Thus this drastic measure, which can also be seen as a means of checking for the importance of issues. Priorities may have changed and if people still would benefit from a feature or fix, the hope is that people will speak up.

In the worst case, a new issue will be created.

[daleyjem]

Personally, I kinda want to go the opposite way and allow a way to error out on circular dependency detection, in which case onwarn would be sufficient. I have no problem with this warning. I'd be on the side that says to keep it.

[lukastaegert]

Not really a "fix" but starting with Rollup@1.27.4, circular dependency warnings will be aggregated by the CLI to reduce the noise. There will be at most five individual cycles displayed, if there are more, it will display the first three and a note that there are more. onwarn handlers will still receive all warnings.

[daleyjem]

Just a note... circular dependencies have caused unexpected consequences for me when preserveModules was set to true (better tree-shakability when a dependency of a Webpack project), as I gathered what seemed to be the result of hoisting not happening the same way as with single-file bundles. Perhaps that was in combination with async import() and code splitting (I don't remember exactly). So I still stand on keeping the warnings there.

At my org... circular dependencies warnings are a code smell and can especially cause issues if ignored when trying to remove the use of deep imports