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

Sass includePaths and Babel-resolved aliases #3

Open
orlin opened this issue Feb 22, 2017 · 16 comments
Open

Sass includePaths and Babel-resolved aliases #3

orlin opened this issue Feb 22, 2017 · 16 comments

Comments

@orlin
Copy link

orlin commented Feb 22, 2017

I can’t get eyeglass to work and that seems like the best way to load sass from node_modules. Perhaps the only way in some cases it seems (when reaching for a path inside a module is necessary). For example trying to load scss from material-components-web fails, very sad. I was hoping eyeglass could help with that and other things. There is an open issue that I just commented on. Also tried the webpack-combine-loaders from the example @wesm87 has there which didn’t help. Any ideas about how to get this working? Look like a very worthwhile addition / a best practice?

@davibe
Copy link
Owner

davibe commented Feb 22, 2017

I don't completely understand your issue.

It seems like you want to load a sass file from inside node_modules directory. If this was possible even without eyeglass would this fix your issue ?

If so please have a look at #2

@orlin
Copy link
Author

orlin commented Feb 22, 2017

Yes, I know about normalize. There is also normalize-scss that I do @import '~normalize-scss’; from node_modules without eyeglass. Eyeglass lets you reach it without a ~ (which may not be supported in webpack 3 thus also would be breaking sass-loader I presume), and eyeglass it seems can import without a relative path, and also with any arbitrary path following the module name.

I just tried a relative path to one of the material-components-web modules: @import '../node_modules/@material/button/mdc-button.scss’; and it doesn’t work - from the pages directory, and neither does @import '~@material/button/mdc-button.scss’; with a ~ rather than relative path. The problem is that further imports from that scss to another of its related modules (notice also without .scss extension) don't work. I guess that material-components-web are also expecting eyeglass-resolved paths or other build tools knowing how to work within node_modules. Here is the error - all the same no matter how I import it:

 error  in ./pages/index.js.scss

Module build failed:
@import "@material/animation/variables";
^
      File to import not found or unreadable: @material/animation/variables.
Parent style sheet: /Users/om/Dev/mel/drmelgill.com/node_modules/@material/button/mdc-button.scss
      in /Users/om/Dev/mel/drmelgill.com/node_modules/@material/button/mdc-button.scss (line 17, column 1)

 @ ./pages?entry 43:15-41
 @ multi ./~/next/dist/client/webpack-hot-middleware-client ./pages?entry

There is also the babel-plugin-module-resolver that could help, I had it planned to try that today / tomorrow. Will let you know how it goes. So I could just import with ‘node_modules/…’ or ‘modules/…’ without knowing the relative distance. That’s pretty cool for other things too like ‘components’, etc. I doubt it will help in the case of sass modules though.

And in any case there is a lot more to eyeglass than imports, and many sass modules on npm supporting it. From memory: there's also support for images, sprites, etc. I think it would be a very worthwhile addition, if someone could figure out how to plug it in...

@orlin
Copy link
Author

orlin commented Feb 22, 2017

Something else: the failing @material/animation/variables that node_modules/@material/button/mdc-button.scss tries to import is actually a node_modules/@material/animation/_variables.scss as I just remembered to check. This is something the babel resolver wouldn't know how to find (the prepended file _) whereas I think that is considered normal in sass context.

@orlin
Copy link
Author

orlin commented Feb 23, 2017

A nice guy from material-components-web helped me solve this issue. So we can let eyeglass worry about its webpack compatibility. If you look at the solution though, I think your next.js example could be improved by setting the sass-loader's includePaths to all node modules. Because we don't depend on any specific modules in the example, all possible paths would be the only thing that makes sense. I think though that what I currently have working presupposes use of yarn, so maybe at least add a comment in the #readme? Here is what changed about my next.config.js so far:

const path = require('path')
const glob = require('glob')

//...

      { test: /\.s(a|c)ss$/,
        use: [
          'babel-loader', 'raw-loader', 'postcss-loader',
          { loader: 'sass-loader',
            options: {
              includePaths: glob.sync('node_modules').map((d) => path.join(__dirname, d))
            }
          }
        ]
      },

@orlin
Copy link
Author

orlin commented Feb 24, 2017

For the record, here is my improved config:

          { loader: 'sass-loader',
            options: {
              includePaths: ['node_modules', 'node_modules/@material/*']
                .map((d) => path.join(__dirname, d))
                .map((g) => glob.sync(g))
                .reduce((a, c) => a.concat(c), [])
            }
          }

The example could come without the 'node_modules/@material/*'- then people would add whatever paths or globs they want to the array.

@davibe
Copy link
Owner

davibe commented Feb 27, 2017

Interesting, could you work on a PR ?

@orlin
Copy link
Author

orlin commented Feb 27, 2017

On this repo or next.js/examples/with-global-stylesheet?

@davibe
Copy link
Owner

davibe commented Mar 1, 2017

i think so, yes

@orlin
Copy link
Author

orlin commented Mar 1, 2017

Sure, I’d love to do my first PR on Next.js, thank you for leading the way! Here is what I intend to include in this one:

  • refactor of next.config.js to use the newer Webpack 2 rules style
  • load either .scss or .sass
  • includePaths for sass-loader - global styles and node_modules
  • move the stylesheets to a /styles dir (so that relative path imports won’t be necessary neither from scss nor from js) and rename them to styles/index.scss and styles/index.css - I’ve been happily doing this at the level of components and pages (what we’re doing here), and I guess at the layout (or even custom _document) level is just as possible for even more global stylesheets (though @include works fine, making further globalization optional) - performance optimization is a separate issue I haven’t thought about yet
  • setup an alias to styles using babel-plugin-module-resolver
  • material-components-web is out of scope as far as this example, but I’ll leave the glob feature with a note in the #readme

I have at least one further improvement in mind regarding PostCSS. Let’s discuss / do that with a separate issue…

@orlin
Copy link
Author

orlin commented Mar 2, 2017

The pull request I just made has one flaw that I can see - example.gif doesn't reflect that the css file has been moved and renamed. Technically it could have stayed in pages the way it was. I just thought it would be better for consistency...

@orlin orlin changed the title eyeglass Sass includePaths and Babel-resolved aliases Mar 2, 2017
@davibe
Copy link
Owner

davibe commented Mar 3, 2017

I will update things accordingly.

@davibe
Copy link
Owner

davibe commented Mar 29, 2017

@orlin i have brought in the changes from next.js but in order to make it work i had to do this
f1ec075

Does this mean the example in the next.js sources is broken ? or what am i missing ?

@orlin
Copy link
Author

orlin commented Apr 13, 2017

@davibe not all the changes. About styles without relative path you'd have to add the following to .babelrc's plugins:

    [
      "module-resolver", {
        "root": ["."],
        "alias": {
          "styles": "./styles"
        },
        "cwd": "babelrc"
    }]

I see you've got babel-plugin-module-resolver in the dependencies, so this should do it...

@orlin
Copy link
Author

orlin commented Apr 13, 2017

Here are the 2 pull requests, if you want to double-check that you copied everything over:

@orlin
Copy link
Author

orlin commented Apr 17, 2017

@davibe I also got a link on the example's readme fixed with this PR to what I originally intended -- Deconfusing Pre- and Post-processing

@coderavels
Copy link

coderavels commented Aug 14, 2020

How can this be achieved in [email protected] without creating custom next.config.js?

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

3 participants