Dynamic imports throwing madge off

[bennypowers]

I've got some fancy footwork importing these modules:

const listSpecifier = './route-list.js';
const restSpecifier = './route-restaurant.js';

const list = [
  './restaurant-card.js',
  './restaurant-filters.js',
  './view-list.js',
  './map-marker.js',
];

const rest = [
  './review-card.js',
];

const imports = ({ list, rest });

const importSpecifier = specifier => import(specifier);

const runDefault = ({ app }) =>
  module => module.default({ app });

const router = async location => {
  const page = location.pathname === '/' ? 'list' : 'rest';
  if (page === 'rest') app.classList.add('restaurant');
  else app.classList.remove('restaurant');
  // Parallelize loading to speed up critical path render
  Promise.all(imports[page].map(importSpecifier));
  return import(page === 'list' ? listSpecifier : restSpecifier)
    .then(runDefault({ app }));
};

Madge pukes on that ternary in the import statement:

  tree traversing /Users/bennyp/Documents/mws-restaurant-stage-1/js/main.js +0ms
  precinct options given:  { includeCore: false, type: undefined } +3ms
  precinct module type:  es6 +4ms
  tree extracted 8 dependencies:  [ './db/cacheRequest.js',
  './router.js',
  undefined,
  undefined,
  '/node_modules/@power-elements/emoji-checkbox/emoji-checkbox.js',
  '/node_modules/@power-elements/lazy-image/lazy-image.js',
  '/node_modules/@power-elements/service-worker/service-worker.js',
  '/bower_components/good-map/good-map.js' ] +5ms
  cabinet found a resolver for .js +6ms
  cabinet reusing the given ast +0ms
  cabinet using commonjs resolver for es6 +0ms
  cabinet adding /Users/bennyp/Documents/mws-restaurant-stage-1/js/node_modules to the require resolution paths +0ms
  cabinet resolved path: /Users/bennyp/Documents/mws-restaurant-stage-1/js/db/cacheRequest.js +0ms
  cabinet resolved path for ./db/cacheRequest.js: /Users/bennyp/Documents/mws-restaurant-stage-1/js/db/cacheRequest.js +0ms
  cabinet found a resolver for .js +0ms
  cabinet reusing the given ast +0ms
  cabinet using commonjs resolver for es6 +0ms
  cabinet adding /Users/bennyp/Documents/mws-restaurant-stage-1/js/node_modules to the require resolution paths +0ms
  cabinet resolved path: /Users/bennyp/Documents/mws-restaurant-stage-1/js/router.js +0ms
  cabinet resolved path for ./router.js: /Users/bennyp/Documents/mws-restaurant-stage-1/js/router.js +0ms
  cabinet found a resolver for .js +0ms
  cabinet reusing the given ast +0ms
  cabinet using commonjs resolver for es6 +1ms
  cabinet adding /Users/bennyp/Documents/mws-restaurant-stage-1/js/node_modules to the require resolution paths +0ms

✖ TypeError: Cannot read property '0' of undefined
    at commonJSLookup (/Users/bennyp/.config/yarn/global/node_modules/filing-cabinet/index.js:208:14)
    at jsLookup (/Users/bennyp/.config/yarn/global/node_modules/filing-cabinet/index.js:163:14)
    at cabinet (/Users/bennyp/.config/yarn/global/node_modules/filing-cabinet/index.js:60:16)
    at Function.module.exports._getDependencies (/Users/bennyp/.config/yarn/global/node_modules/dependency-tree/index.js:105:20)
    at traverse (/Users/bennyp/.config/yarn/global/node_modules/dependency-tree/index.js:149:37)
    at module.exports (/Users/bennyp/.config/yarn/global/node_modules/dependency-tree/index.js:34:19)
    at files.forEach (/Users/bennyp/.config/yarn/global/node_modules/madge/lib/tree.js:116:27)
    at Array.forEach (<anonymous>)
    at Tree.generateTree (/Users/bennyp/.config/yarn/global/node_modules/madge/lib/tree.js:111:9)
    at <anonymous>

Thanks for the cool module!

[mrjoelkemp]

Thanks for reporting the issue. I'm afraid that the fancy footwork is defeating static analysis here :) The only way for dependency-tree to recognize those imports is to evaluate the code to inline the assignments. Maybe facebook's prepack could be a good prepass filter for dependency-tree; not sure if anyone has tried that.

[devinrhode2]

So... work-arounds... simplest that has worked for me is to temporarily re-write code so that it does not use dynamic imports... Maybe there could be a codemod that does this for you just before madge receives it... but, sure is hard if the dynamic imports are determined at runtime. All the dynamic code may need to be commented out/undone, which shouldn't really take all that long.

The webpack chunk names don't really map to the import paths...

What do people think of a codemod?

[devinrhode2]

Of course, a codemod might work best if the arguments to import( are static (not determined at runtime)

This is a lot easier to handle:

const FooPage = () => import('./page/Foo')

Than:

// some crazy stuff happens
pages[page] = import(page)

The first snippet I'd think mostly anyone could write some code to find import( and work some magic in any language of their choosing, although it'd be ugly, obviously a codemod is the way to go. I'd imagine a codemod is an absolutely do-able thing. For now, I'm fine with hand-converting the few imports that I have, stashing that change in git, generating an svg, and when I want to re-generate that svg, I can just git stash pop.

[phaux]

Looks like it was fixed in detective-typescript v7.0.0:

https://github.com/pahen/detective-typescript/commit/5977a3932d7e8a7168c9a3a4b0ab0c5334d9a9ee

[vidal7]

I think that just upgrading precinct dependency to version 8 might fix this issue. There is alreay a pull request pending https://github.com/pahen/madge/pull/278.

[vidal7]

I found a workaround (https://www.npmjs.com/package/npm-force-resolutions) for now but ideally, precinct should be upgraded to upgrade detective-typescript to version 7.0.0

[fdc-viktor-luft]

@vidal7 @bennypowers does this issue still exist on the latest version?

[DexterHuang]

Guys here is the solution for this I found: in your .madgerc config file you include

{
	"detectiveOptions": {
		"ts": {
			"skipTypeImports": true,
			"skipAsyncImports": true
		},
		"tsx": {
			"skipTypeImports": true,
			"skipAsyncImports": true
		}
	}
}

this way it will ignore the type imports and async imports, works for me :) hope this helps. My personal opinion is that the point for checking circular dependency is to ensure there won't be problems caused during module loading, skipping types (since they don't compile to actual code), or skipping async import (people might disagree on the cleanliness or if we are "supposed" to use async import to fix circular dependency), will fix circular dependency caused issues (at least to my limited knowledge); maybe these two should be on by default.