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

PostCSS block with SASS and sourceMap #116

Closed
kirill-konshin opened this issue Feb 16, 2017 · 30 comments
Closed

PostCSS block with SASS and sourceMap #116

kirill-konshin opened this issue Feb 16, 2017 · 30 comments
Labels

Comments

@kirill-konshin
Copy link
Contributor

Adding sass({sourceMap: true}) produces error:

TypeError: Path must be a string. Received undefined
@andywer
Copy link
Owner

andywer commented Feb 16, 2017

Hmm, works on my machine...

Could you please provide the stack trace, the complete config and the package versions you use?

@kirill-konshin
Copy link
Contributor Author

    throw new TypeError('Path must be a string. Received ' + inspect(path));
    ^

TypeError: Path must be a string. Received undefined
    at assertPath (path.js:7:11)
    at Object.relative (path.js:1226:5)
    at Object.onRender [as callback] (/Users/x/Sites/webpack-blocks/node_modules/sass-loader/index.js:282:42)
    at Object.<anonymous> (/Users/x/Sites/webpack-blocks/node_modules/async/dist/async.js:2234:31)
    at apply (/Users/x/Sites/webpack-blocks/node_modules/async/dist/async.js:20:25)
    at Object.<anonymous> (/Users/x/Sites/webpack-blocks/node_modules/async/dist/async.js:56:12)
    at Object.callback (/Users/x/Sites/webpack-blocks/node_modules/async/dist/async.js:840:16)
    at options.success (/Users/x/Sites/webpack-blocks/node_modules/node-sass/lib/index.js:303:32)

Sass file used for import:

$icon-font-path: '~bootstrap-sass/assets/fonts/bootstrap/';
@import '~bootstrap-sass/assets/stylesheets/bootstrap';
body.foobarbaz {
    background: gray;
}

Versions:

    "@webpack-blocks/extract-text2": "^0.4.0",
    "@webpack-blocks/postcss": "^0.4.1",
    "@webpack-blocks/sass": "^0.4.0",
    "@webpack-blocks/webpack2": "^0.4.0",
    "autoprefixer": "^6.7.2",
    "webpack": "^2.2.1",
    "bootstrap-sass": "^3.3.7",

Config:

    sourceMaps(),
    sass({relative_assets: true, sourceMap: true}),
    extractText('[name].css', 'text/x-sass'), // change to x-less if less is used

@andywer
Copy link
Owner

andywer commented Feb 16, 2017

Seems to be an issue with the webpack sass-loader: webpack-contrib/sass-loader#285

Upgrading to sass-loader v6.0.0 might solve the issue, but we cannot easily do that, since it is incompatible with webpack@1...

Another possible fix: Try setting the webpack context path (as suggested in the sass-loader issue).

@kirill-konshin
Copy link
Contributor Author

I've added the following and it fixed the issue:

    addPlugins([
        new webpack.LoaderOptionsPlugin({
            options: {
                context: '/'
            }
        }),
    ])

@andywer
Copy link
Owner

andywer commented Feb 17, 2017

It's even shorter if you use setContext 😉

-    addPlugins([
-        new webpack.LoaderOptionsPlugin({
-            options: {
-                context: '/'
-            }
-        }),
-    ])
+    setContext('/')

The question is now: How do we handle this issue in webpack-blocks? ... :-/

Might bump the sass-loader to v6.0.0 for webpack@2 as soon as webpack-blocks 1.0/2.0 is out (see here).

@kirill-konshin
Copy link
Contributor Author

kirill-konshin commented Feb 17, 2017

setContext did not help as it's a different thing in Webpack 2. It was first thing I tried...

@andywer
Copy link
Owner

andywer commented Feb 17, 2017

Ahh, really? Shit. Cannot find anything about that in the official upgrade docs :-/

Do you know any document stating what they changed about the context?

@kirill-konshin
Copy link
Contributor Author

https://webpack.js.org/guides/migrating/#loaderoptionsplugin-context it's down there ))) in the doc you just sent.

@andywer
Copy link
Owner

andywer commented Feb 17, 2017

Thanks for the link! But it doesn't state that the context prop is gone...

Did they drop the context prop altogether? Can't find it in the new docs.

@kirill-konshin
Copy link
Contributor Author

Context is still there, not dropped, at least to my knowledge... but those are two different things.

@kirill-konshin
Copy link
Contributor Author

Any resolution? I suggest to add a plugin automatically if it's Webpack 2 and sourceMap.

@andywer
Copy link
Owner

andywer commented Feb 22, 2017

@kirill-konshin Created a bugfix branch. Could you please try this fix? b170df7

@kirill-konshin
Copy link
Contributor Author

Works fine. Btw, LoaderOptionsPlugin can also have {context: process.cwd}, I haven't figured what's the difference, but I think simple / should be just fine.

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

Strange that I just cannot reproduce it locally, though...

@kirill-konshin
Copy link
Contributor Author

https://github.com/kirill-konshin/react-ssr-playground/tree/master/webpack-blocks you can use my playground, it has the less block from #114 where I checked the behavior too.

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

Still works if I use your playground and comment-out the LoaderOptionsPlugin... 🤔

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

(Used npm run server-dev)

@kirill-konshin
Copy link
Contributor Author

It's because this plugin is now in lessBlock.js )))

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

Ahhh... Here we go 🙈

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

But I still cannot get the sass/extract-text end-to-end test to fail without the fix. Cannot see the major difference to your playground code 🙃

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

(I added the sourceMaps() block and sourceMap: true to the sass block; tried with and without extract-text)

bildschirmfoto 2017-02-24 um 20 01 19

@kirill-konshin
Copy link
Contributor Author

I have pushed an update to playground, it consistently fails on npm run build...

@kirill-konshin
Copy link
Contributor Author

kirill-konshin commented Feb 24, 2017

I commented out everything unrelated and narrowed to these lines:

postcss([
    autoprefixer({browsers: ['last 2 versions']})
]),

So seems probably not SASS/LESS should have additional plugin, but postcss :)

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

Oh my god... 😅

But really seems like it. Also found another issue comment pointing in a similar direction.

Now I'm asking myself: Is the sass block the correct place to fix it or should the fix go into the postcss block? 🤔

@ai Do you know anything about this weird issue?
(Short summary: sass-loader + source maps + postcss-loader + webpack 2 = "TypeError: Path must be a string. Received undefined", if you don't set a context path using the LoaderOptionsPlugin)

@kirill-konshin kirill-konshin changed the title SASS block and sourceMap PostCSS block with SASS and sourceMap Feb 24, 2017
@kirill-konshin
Copy link
Contributor Author

Probably postcss block is a better place for the fix since it is the root cause.

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

Is it the root cause? I am not sure, since both the postcss and the sass block work perfectly fine if used without the other one. And even together they work unless you enable the sass source maps...

@ai
Copy link

ai commented Feb 24, 2017

Never saw this error, sorry 😞

@kirill-konshin
Copy link
Contributor Author

Well... Sass + source map (w/o postcss) works OK, so it's kinda self contained. If we add postcss to the mix, then it fails. Btw, if you add plugin to SASS, you also have to add it to LESS, so if both are used together (crazy) then plugin will be added twice. So postcss seems to be a better and more semantic location...

@andywer
Copy link
Owner

andywer commented Feb 24, 2017

@ai No problem :)

@kirill-konshin Good point! 👍

andywer pushed a commit that referenced this issue Feb 25, 2017
andywer pushed a commit that referenced this issue Feb 25, 2017
@kirill-konshin
Copy link
Contributor Author

Woohoo! Thanks!

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

No branches or pull requests

3 participants