Does not get correct dependencies when mixing es6 and cjs import syntax

[pcardune]

I'm working on a rather large code base that is slowly transitioning from es5 to es6. There are a lot of files that have mixed imports: i.e. both require() statements and import statements. node-precinct does not correctly extract all the dependencies.

For example, given the file:

require('./foo');
import './bar';

only the foo dependency will be recognized.

If you flip it around, and have a file like:

import './bar';
require('./foo');

only the bar dependency will be recognized.

So in otherwords, precinct use looks at the first syntax it encounters and uses that for the rest of the file, even though the file may have a mix of cjs and es6.

[mrjoelkemp]

Hey @pcardune! Thanks for filing the issue! Out of curiosity, for what purpose are you using precinct?

I can think of a few ways of approaching the mixed import case:

1. You could use Lebab's commonjs transform to turn CJS requires into ES6 imports https://github.com/lebab/lebab#safe-transforms.
2. I could look into extending detective-es6 with the functionality detective-cjs (since mixed codebases are likely until we have a proper es6 loader). This is hairy since I need to juggle a few things: preserving the separation of import concerns, avoiding double traversals of an ast, and finding a sane runtime extension point to combine the checks for es6 and cjs.

While I'm happy to make this lib more robust, I'd prefer to find a decent workaround until more folks request support for this use case. Thoughts?

[pcardune]

I'm actually not using precinct directly. I'm using madge to generate svg dependency graph images. madge uses dependency-tree, which uses precinct, which lead me to this repo :)

I'm also thinking about using dependency-tree to power a linter that enforces rules about how modules in different directories can depend on each other.

I will check out lebab and get back to you. I previously tried using some transforms for jscodeshift to convert require() calls, but alas, we have some nested require() calls and the transform doesn't handle them properly :( :(. Hopefully lebab works better...

[pcardune]

@mrjoelkemp

Unfortunately lebab does not manage to figure out when to use

import * as foo from './foo-module'

instead of

import foo from './foo-module'

so converting all the code would require a lot of manual work :( :(

Here is an example of a file that I am working with: https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/applab/applab.js

[mrjoelkemp]

Thanks so much for the details @pcardune! I'll pull down that repo and use it as a performance testbed for the change to run both the commonjs and es6 detectives for CJS or ES6 files. I'll also continue to think through some alternative solutions.

I'm a big fan of what code.org is doing, so I'm happy to work on a solution. I'll keep you posted on the progress; though I foresee it taking a couple of nights of work to land on a fully tested solution that achieves a lot of the goals I've mentioned earlier.

@pahen any additional thoughts on a path forward that doesn't introduce performance regressions?

[pahen]

Hmm, that was a tricky one. I suppose it would involve a lot of work and changes to support mixed import syntaxes @mrjoelkemp ? I'm afraid I can't think of any easy fix for this that doesn't introduce performance regressions :(

[pcardune]

FWIW, I would be willing to take performance regressions to make this work. Perhaps it would be possible to add an option that explicitly enables mixed import syntaxes, which third parties could then choose to enable/disable depending on their needs?

[mrjoelkemp]

Fair point @pcardune! The goal of precinct is zero configuration, but i like the opt-in perf regression for transitionary codebases. Will work on that and support the option all the way up to madge.

[mrjoelkemp]

@pcardune I added an option es6.mixedImports that will chain the es6 and cjs detectives. See the usage docs for the syntax.

You can get this with a fresh install of madge, but I'll still bump precinct within dependency-tree.

Thanks again for suggesting the feature. Let me know if you run into any issues.

[NickHeiner]

Can we get the same mixed imports functionality for TS? I'm in a codebase that's migrating from JS to TS, and we have some cases where we still need to use CJS.