Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autosuggest.scss could not be resolved. #1

Closed
QuentinAndre opened this issue Oct 5, 2016 · 9 comments
Closed

Autosuggest.scss could not be resolved. #1

QuentinAndre opened this issue Oct 5, 2016 · 9 comments

Comments

@QuentinAndre
Copy link

Hi there,

First of all, great component, it looks absolutely amazing, but I cannot get it to work. I have sass-loader installed, as well as all the dependencies, import the library using RequireJS (import Autosuggest from 'react-bootstrap-autosuggest') but still get the following error when compiling with webpack:

Module not found: Error: Cannot resolve module 'Autosuggest.scss'

After editing the require('Autosuggest.scss'); line in the lib/Autosuggest.js and replacing it by a relative reference require('../src/Autosuggest.scss'), the module can now be found, but compilation still fails when trying to import the styles from Bootstrap:

ERROR in ./~/css-loader!./~/sass-loader!./~/react-bootstrap-autosuggest/src/Autosuggest.scss Module build failed: @import "bootstrap/variables"; @import "bootstrap/mixins";

Any clue how to solve the issue? I'd really love to use your component....

@viridia
Copy link

viridia commented Oct 6, 2016

I was able to get your first problem working by using a custom webpack resolver:

  new webpack.NormalModuleReplacementPlugin(
    /Autosuggest\.scss$/,
    'react-bootstrap-autosuggest/src/Autosuggest.scss')

For the second part, I suspect he's using the sass versions of the bootstrap styles - you'll need to add a dependency to bootstrap-sass instead of the normal boostrap css which uses less.

@QuentinAndre
Copy link
Author

QuentinAndre commented Oct 6, 2016

Hi Viridia,

Your fix solved the first issue for me, indeed.

I figured out the second issue: the package bootstrap-sass-loader was deprecated, and I needed to use sass-loader and sass-resources-loader instead.

Thank you for pointing me in the right direction!

EDIT: I still had issues with react-bootstrap-autocomplete styles not being merge with react-bootstrap style. I fixed the issued by adding { test: /\.(sass|scss)$/, loaders: ["style", "css", "sass?includePaths[]=./node_modules/bootstrap-sass/assets/stylesheets"] }, to the webpack.config file.

@trevorr
Copy link
Collaborator

trevorr commented Oct 27, 2016

As you've discovered, the style import is intended to require some external configuration in your module loader (such as Webpack). I could refrain from importing it at all, and leave that to the applications to do, but that seemed likely to confuse people even more, since it would build but then be unstyled. I'm certainly open to other ways of handling this.

And yes, I use bootstrap-sass instead of bootstrap because a) I think Sass is a slightly more powerful and/or easy to use and b) Twitter is moving to Sass for Bootstrap 4. Since Autosuggest.scss uses variables and mix-ins defined in bootstrap-sass, it needs access to the source Sass files.

Anyway, there are various ways to get Webpack to find all the necessary files. Here's an excerpt of the relevant parts from one of my webpack.config.babel.js files:

import ExtractTextPlugin from 'extract-text-webpack-plugin'
import path from 'path'
module.exports = {
  entry: [ 'bootstrap-loader', path.join(__dirname, 'src') ],
  plugins: [
    new ExtractTextPlugin('styles.css', { allChunks: true })
  ],
  module: {
    loaders: [
      {
        test: /\.scss$/,
        loader: ExtractTextPlugin.extract('style', 'css!sass?includePaths[]=./node_modules/bootstrap-sass/assets/stylesheets')
      }
    ]
  },
  resolve: {
    alias: {
      'Autosuggest.scss': path.resolve(__dirname, 'node_modules/react-bootstrap-autosuggest/src/Autosuggest.scss')
    }
  }
}

Also, here are the relevant node dependencies for that project:

$ grep 'sass\|bootstrap\|epend.*:' package.json
  "devDependencies": {
    "bootstrap-loader": "^1.0.10",
    "node-sass": "^3.7.0",
    "sass-loader": "^3.2.0",
  "dependencies": {
    "bootstrap-sass": "^3.3.6",
    "react-bootstrap": "^0.29.4",
    "react-bootstrap-autosuggest": "^0.4.1",
    "react-router-bootstrap": "^0.23.0",

@trevorr trevorr closed this as completed Oct 27, 2016
@joekrill
Copy link

I realize this is closed, but wouldn't it make more sense to update the package.json to point to the dist folder instead of the lib folder? That way all of this stuff isn't even necessary?

...
  "main": "dist/react-bootstrap-autosuggest.js",
...

Otherwise, why even bother building a dist bundle anyway?

@trevorr
Copy link
Collaborator

trevorr commented Jan 10, 2017

@joekrill: The dist bundle is only intended to be used directly by the browser, perhaps distributed via a CDN. For example:

<script src="path-to-dist/react-bootstrap-autosuggest.min.js"></script>

If you're using your own bundling tool, such as Webpack, you usually don't want to include already bundled dependencies, as their transitive dependencies will be duplicated (except the ones marked as external, like React). The lib folder is what you want in that case. As a somewhat separate issue, it doesn't inject the styles for you because you may wish to customize the styles or how they're loaded. While it makes getting started a little trickier, real applications tend to need that level of control.

@joekrill
Copy link

@trevorr That makes sense. It's just unusual to see a module packaged in a way that requires your loader handle being able to require scss files (even if that entails just telling your loader to ignore them). I'm fairly adept with webpack and still had a difficult time getting my webpack config to ignore the styles altogether, due to a combination of factors.

One of those factors I think is actually a bug/problem with the /lib build. /lib/Autosuggest.js includes this line:

require('Autosuggest.scss');

Which comes from this line in /src/Autosuggest.js:

import 'Autosuggest.scss'

I can't quite figure out if this is intentional. Surely that import should actually be:

import './Autosuggest.scss'

No?

@trevorr
Copy link
Collaborator

trevorr commented Jan 13, 2017

@joekrill I believe it was intentional, so that it would be easier to customize its handling by the loader. For instance, a customized version in an application could be selected just by having it in the application's source path(s). The ./ prefix would require specifically overriding the default loader behavior for that file, which I was concerned could be problematic for some loaders. (Webpack can handle it, but you have fewer options for how.)

Basically, I think it comes down to how likely applications are to override it. My assumption (based on my own applications) was that it would be common. By requiring it in the source, I make sure the styles are loaded. (Otherwise, I'm sure there would be lots of questions about why it looks completely broken because there are no styles.) It would be nice if it would "just work out of the box" without loader configuration, but I'd want to find a way that doesn't make it more difficult to make the customizations that most people are likely going to need eventually.

(To be honest, since this was my first big React component, I probably also found other components that worked this way, and followed their lead. Unfortunately, that was long enough ago that I can't remember which ones.)

@joekrill
Copy link

@trevorr If an alternative component were to come to the same conclusion, though, then there's a huge problem, because both would be requiring the same "global" module ('Autosuggest.scss', in this case), and there would be no way to distinguish between the two. Is that highly unlikely? Probably. But "Autosuggest" isn't exactly highly unique, either.

But ignoring that problem, I'm running into an issue where I'm testing components with node and mocha. No webpack or anything here -- just using babel. So I can't even intercept Autosuggest.scss unless I override NODE_PATHand create a blank Autosuggest.scss file, otherwise node just totally bails because it can't even find the file. If it were relative, I could just tell babel to look elsewhere. But there's no way to do that when it's a global require like this.

@trevorr
Copy link
Collaborator

trevorr commented Apr 18, 2017

The consensus seemed to be that Autosuggest.scss shouldn't be required automatically, so I've removed that behavior in the 0.5 release. Thanks @joekrill @QuentinAndre @viridia for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants