Use consistent, specific imports

[marshallswain]

This updates the entire package with consistent imports. Updating the imports to be file-specific instead of using the folder shorthand ensures compatibility with all module loaders.

[levithomason]

Initially, I'm not in favor of such a change. Although, I'd be curious to hear the motivation for this.

---

FWIW, there are also some errors in the updates, such as TextArea/index.js:

export default from './TextArea/TextArea'

./TextArea is a JS file, so there is no extra directory here.

[levithomason]

It would also appear we have neither linting nor tests around the index files :)

[marshallswain]

The motivation behind this is to allow the package to load in browser-based module loaders like StealJS, which is based on SystemJS. I figured making a PR is the best solution to making it work everywhere. And while the loaders do allow you to create a configuration map of the files, I was hoping to avoid doing that. I'm also hoping to avoid having to switch back to Webpack to use the package.

If I change the imports to go through the index.js files and audit/clean up the mistakes, are you open to allowing the change? Thank you!

[marshallswain]

I've cleaned up those mistakes and switched to using the index.js files. Please let me know if there's anything I can do that would help you feel more comfortable merging this.

[codecov-io]

Current coverage is 95.89% (diff: 100%)

Merging #1221 into master will not change coverage

@@             master      #1221   diff @@
==========================================
  Files           880        880          
  Lines          4901       4901          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           4700       4700          
  Misses          201        201          
  Partials          0          0          

Powered by Codecov. Last update 935fd95...7b393b8

[levithomason]

I certainly want maximum interop for the lib. My ignorance is exposed regarding browser module loaders. I'll take a look at those and review this PR in light of that. Thanks for the work and extra explanation here.

[levithomason]

Note for the future. PRs opened from a branch on your fork will allow myself and the collaborators here to push updates to it. Though, this is not possible against your master branch. This is handy for fixing conflicts, lint issues, and tests, if needed.

No need to reopen a new PR, just a note for the future 👍 Thanks again!

[jeffcarbs]

I'm still not sure what problem this PR is solving. This project uses webpack internally as a build tool and the output of that is pushed up to npm in both UMD and commonjs formats. It seems that stealjs supports commonjs (https://stealjs.com/docs/index.html#the-module-loader) so it seems like it should work in its current state.

How module importing is done internally within the semantic-ui0-react repo should not affect how it's used elsewhere.

[marshallswain]

This is the issue I'm trying to resolve. When I try to use the CommonJS or ES6 formats of the module, I get the errors in this screenshot. I'll file an issue with StealJS, too.

[matthewp]

Hi, I'm the maintainer of StealJS. As I understand from this discussion, the package.json main of dist/commonjs/index.js is not meant to be loaded in a browser. Given that, you could fix this by including a browser configuration that points to the umd build: "browser": "dist/umd/semantic-ui-react.min.js" should do it.

[levithomason]

I've previously implemented the browser field, #702, however, this immediately broke several users' webpack builds, #703. When creating a bundle with Webpack, requiring semantic-ui-react results in the UMD build being bundled, which is of course not what you want when creating a bundle on the server. Hence, it was quickly reverted, #704.

I'd like to get this going, though, as it stands it seems there is either not full community agreement on the browser field spec, or, I am misunderstanding some part of it.

[matthewp]

Ah, I follow, webpack gives preference to the browser field. So we need a field that webpack will ignore. We actually have one for steal: "steal": { "main": "path/to/browser/main.js" } but I can understand if you don't want to implement a loader-specific configuration. I wish there was an accepted field that webpack left alone.

[levithomason]

Indeed, I'll take a look around and see if anyone has solved this yet. As far as webpack's implementation is concerned, the browser field is for isomorphic versions of your modules that are bundler agnostic.

Without a clear spec nor solution at hand, I'm going to close this for now. It is not pretty, but as a workaround @marshallswain, you can import the full file path:

import Container from 'semantic-ui-react/dist/commonjs/elements/Container'

I'd also suggest discussing ES module support with @matthewp. We provide an es build which can take advantage of tree shaking (i.e. Rollup, Webpack 2, etc).

[marshallswain]

Thanks @levithomason. I'll submit a PR for the mixed module syntax in the debug.js file. That's preventing me from being able to use it as you suggested above.