0

I have the following modules structure:

/components
├── Button.js
├── Checkbox.js
├── index.js
├── DateSelect
    ├── DateSelect.js
    └── index.js

With /components/DateSelect/index.js:

import DateSelect from './DateSelect';

export default DateSelect;

/components/index.js:

import DateSelect from './DateSelect';
import Button from './Button';
import Checkbox from './Checkbox';

export {
  DateSelect,
  Button,
  Checkbox,
};

And /components/DateSelect/DateSelect.js:

import { Checkbox } from '../';
// ...code

// I want to do this!
const MyCustomCheckbox = props => <Checkbox style={someStyle} />;

// ...code
class DateSelect extends React.Component {
  // code
}
export default DateSelect;

Now, I want to access Checkbox as in the code above, in the top level scope of the file, but I get undefined. If I access this variable, however, in the render method of DateSelect, it works as expected.

I'm not completely sure on why this is the case, or how I can fix this (I can do import Checkbox from '../Checkbox', but I don't want to change the pattern of using the index.js file of the directory), and I also want to understand exactly what's going on. Any ideas?

YoTengoUnLCD
  • 600
  • 7
  • 15
  • Why does `Checkbox.js` depend on `Dateselect.js`? Can you post its code please? – Bergi Oct 31 '17 at 17:12
  • @Bergi sure, give me a few minutes to get to my computer, but checkbox doesn’t depend on DateSelect, the circular dependency I talked about was between DateSelect.js and components/index.js, importing modules from each other. – YoTengoUnLCD Oct 31 '17 at 17:14
  • 1
    Well, that does in turn make `Checkbox` depend on the imported `Dateselect` as well. [Avoid such circular references](https://stackoverflow.com/a/46593566/1048572)! Just import the `Button` module directly. – Bergi Oct 31 '17 at 17:15
  • Yep @Bergi is right - just import `Checkbox` directly. Using the `index` pattern is great for defining internal APIs and maintaining strict module boundaries but you can't refer back to it from a component which it imports from. If you want to stick to the index pattern, break up the `Checkbox` file into a mini module `/components/CheckBox/Checkbox.js` and `/components/CheckBox/index.js` and import from that index file. – Marc Davies Oct 31 '17 at 17:23
  • Thanks Bergi and @MarcDavies! I have one question though, as I’d like to keep the index pattern, why would moving Checkbox to a mini module like that fix the issue? I’m not really clear on why my DateSelect component works at all having those circular dependencies haha. – YoTengoUnLCD Oct 31 '17 at 18:03
  • OK @YoTengoUnLCD I'll write it as an answer as it is difficult to format in this comment block. – Marc Davies Nov 01 '17 at 11:20

1 Answers1

1

Your problem is that you have a circular dependency on the export in /components/index.js.

When your build tool first builds your application, it does the following...

  • starts at, say, app.js, which imports DateSelect from components/index.js
  • goes to components/index.js which declares its export

    // SPOILER: This guy is your problem. We'll call him the douche.
    export {
      DateSelect,
      Button,
      Checkbox,
    };
    
  • for the douche to be declared, it needs to import stuff from components/DateSelect.js, components/Button.js and components/Checkbox.js

  • goes to these files and starts building what is in them
  • as soon as it goes to build DateSelect, it is told to import CheckBox from components/index.js
  • it already knows about the douche in components/index.js, so it goes looking for CheckBox on the douche (it 'circles' back to it), but CheckBox has not been declared in components/CheckBox.js yet, so it returns undefined

This is a matter of timing. CheckBox will be defined on the douche a few clicks after we try to import it, but that's a few clicks too late for your build.

You can show this by logging CheckBox immediately and a few milliseconds after the import statement...

// /components/DateSelect/DateSelect.js:

import { Checkbox } from '../';
console.log(CheckBox); // undefined
setTimeout(() => console.log(CheckBox), 4) // function CheckBox(_ref) { ...

So, when you try to declare MyCustomCheckbox, CheckBox is undefined.

By the time DateSelect's render method has been called, CheckBox has been declared. This is, relatively, waaay later than the build process.

There is no 100% safe solution to this kind of problem as it depends on your application structure as a whole. It is usually best to avoid circling back to the same module

Avoid the douche

You can avoid circling back to the douche by importing directly from components/CheckBox.js or take the indexed module approach one level deeper and create a little CheckBox module...

/components
  /CheckBox
    CheckBox.js
    index.js

...then import {CheckBox} from '/components/Checkbox'

Marc Davies
  • 256
  • 1
  • 8
  • The douche is *not* an object. While it looks similar to an object literal, it just a declaration of the exported names. The only difference to your suggestion (`export {DateSelect} from './DateSelect';`) is that's it's equivalent to `export {default as DateSelect} from './DateSelect';`, not `export {DateSelect as DateSelect} from './DateSelect';`. – Bergi Nov 01 '17 at 12:32
  • OK @Bergi so I misunderstood the nature of the douche under the hood. Will edit that out. But it is true that using the douche method creates an export entity which has a stable identity which the build tool circles back to. So - the approach of using individual export statements works. I'm also not sure why you criticise the named export method - it's clearly cleaner. – Marc Davies Nov 01 '17 at 12:52
  • @Bergi Is it appropriate/helpful to downvote an answer which responds directly to the question and offers working solutions but which makes a small technical error? In any case - the implementation of module loading isn't exactly transparent so I don't think referring to exports as objects for this kind of explanation is too great a crime. – Marc Davies Nov 01 '17 at 13:02
  • I'm not sure how far your misunderstanding goes, you get some things right and others (minor ones?) not, yet you arrive at a wrong solution. Thanks for fixing the term object, the next inaccuracy would be "*but `CheckBox` has not been declared yet*" - actually it has been declared (that's why it's found there), but it's not yet *initialised*. I've dowvoted mostly for the first "solution", which can't really work - it doesn't make a difference to change the re-export syntax. It still circles back to the `index.js` module, all you changed was default export vs named export. – Bergi Nov 01 '17 at 21:55
  • @Bergi . 1. re instantiation vs declaration. I'm using 'declare' here to refer to the whole declaration -> instantiation -> assignment flow. Technically, `CheckBox` has been both declared and instantiated as undefined but not yet assigned. I think this level of detail is more distracting than helpful for this answer so I think using declare is appropriate. 2. Having done a bit more testing on my 'Kill the douche' solution, there's a bit of a gotcha in there. It does make a difference to change the re-export syntax. BUT what matters is the ordering of the re-export statements... – Marc Davies Nov 02 '17 at 10:04
  • 1
    ...When I had it working, I happened to have the statements ordered in a way which meant that `CheckBox` was fully declared (declared + instantiated + assigned) in time. When you switch the order, this doesn't happen in time. In any case, it's not a stable solution so I'll edit out. – Marc Davies Nov 02 '17 at 10:05
  • Yes, the ordering of the `import` (or `export from`) statements is the relevant one, as it determines in which order the cycle is resolved. It's a fine solution, but a fragile one. (Depending on the exact dependency graph, not necessarily less fragile than importing the module directly by avoiding the index) – Bergi Nov 02 '17 at 10:06
  • Yes - but even if you order the `import` statements well, using a single export statement breaks it. In any case - importing direct from `CheckBox.js` (or another index file) is way safer. – Marc Davies Nov 02 '17 at 10:08