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

Suggestion: wrap compound "&" #{&} with interpolation to avoid invalid sourcemaps on libsass #1948

Closed
leocaseiro opened this issue Jan 12, 2018 · 4 comments

Comments

@leocaseiro
Copy link

leocaseiro commented Jan 12, 2018

Bugs

Follow the template below to ensure the quickest and most accurate response to your issue.

What MDC-Web Version are you using?

{
    "@material/animation": "^0.25.0",
    "material-components-web": "^0.28.0"
}

What OS are you using?

macOS Sierra 10.12.6

What are the steps to reproduce the bug?

There's a compound selector &--theme-dark in the mdc-theme-dark mixin that breaks sourcemaps within libsass:

@mixin mdc-theme-dark($root-selector: null, $compound: false) {
  @if ($root-selector) {
    @at-root {
      @if ($compound) {
        #{$root-selector}--theme-dark#{&},
        .mdc-theme--dark & {
          @content;
        }
      } @else {
        #{$root-selector}--theme-dark &,
        .mdc-theme--dark & {
          @content;
        }
      }
    }
  } @else {
    &--theme-dark, // <-------- here
    .mdc-theme--dark & {
      @content;
    }
  }
}

I have a repo to reproduce the error.

As a workaround, if we wrap the compound selector with interpolation, the sourcemap is generated successfully.

From & to #{&}

What is the expected behavior?

If we wrap the selector with #{&} we avoid the libsass issue.

What is the actual behavior?

As an example, if we use webpack and run node-sass with sourcemaps enable, libsass breaks the build.

Error:

webpack: Compiling...
^C/Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/node_modules/source-map/lib/source-map-generator.js:298
      throw new Error('Invalid mapping: ' + JSON.stringify({
      ^

Error: Invalid mapping: {"generated":{"line":2076,"column":2},"source":"/Sites/github/material-web-webpack-sass-bug/node_modules/@material/theme/_mixins.scss","original":{"line":128,"column":-2},"name":null}
    at SourceMapGenerator_validateMapping [as _validateMapping] (/Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/node_modules/source-map/lib/source-map-generator.js:298:13)
    at SourceMapGenerator_addMapping [as addMapping] (/Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/node_modules/source-map/lib/source-map-generator.js:110:12)
    at /Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/node_modules/source-map/lib/source-node.js:351:13
    at SourceNode_walk [as walk] (/Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/node_modules/source-map/lib/source-node.js:230:9)
    at SourceNode_walk [as walk] (/Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/node_modules/source-map/lib/source-node.js:226:13)
    at SourceNode_walk [as walk] (/Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/node_modules/source-map/lib/source-node.js:226:13)
    at SourceNode_toStringWithSourceMap [as toStringWithSourceMap] (/Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/node_modules/source-map/lib/source-node.js:342:8)
    at ConcatSource.proto.sourceAndMap (/Sites/github/material-web-webpack-sass-bug/node_modules/webpack-sources/lib/SourceAndMapMixin.js:30:32)
    at getTaskForFile (/usr/local/lib/node_modules/webpack/lib/SourceMapDevToolPlugin.js:33:30)
    at chunk.files.forEach.file (/usr/local/lib/node_modules/webpack/lib/SourceMapDevToolPlugin.js:91:21)
    at Array.forEach (<anonymous>)
    at /usr/local/lib/node_modules/webpack/lib/SourceMapDevToolPlugin.js:89:18
    at Array.forEach (<anonymous>)
    at Compilation.<anonymous> (/usr/local/lib/node_modules/webpack/lib/SourceMapDevToolPlugin.js:88:12)
    at Compilation.applyPlugins1 (/usr/local/lib/node_modules/webpack/node_modules/tapable/lib/Tapable.js:75:14)
    at self.applyPluginsAsync.err (/usr/local/lib/node_modules/webpack/lib/Compilation.js:670:11)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! bug-webpack-sass-source-map-material-components-web@1.0.0 start: `webpack-dev-server --open`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the bug-webpack-sass-source-map-material-components-web@1.0.0 start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/leocaseiro/.npm/_logs/2018-01-12T04_50_57_116Z-debug.log

Please describe what the component/code is actually doing that's different from what it should be
doing.

In my repo, if you clone, install the npm packages and run npm run start. You'll be able to reproduce the error.

To fix it, I've wrapped the compound & inside the @mixin mdc-theme-dark:

@mixin mdc-theme-dark($root-selector: null, $compound: false) {
  @if ($root-selector) {
    @at-root {
      @if ($compound) {
        #{$root-selector}--theme-dark#{&},
        .mdc-theme--dark & {
          @content;
        }
      } @else {
        #{$root-selector}--theme-dark &,
        .mdc-theme--dark & {
          @content;
        }
      }
    }
  } @else {
    #{&}--theme-dark, // <---- here it is
    .mdc-theme--dark & {
      @content;
    }
  }
}

PS: I know this is not related to mdc, but it would be nice to avoid this issue while the libsass team find a way to fix it. It seems pretty hard to fix it there.

leocaseiro added a commit to leocaseiro/material-components-web that referenced this issue Jan 12, 2018
@leocaseiro leocaseiro changed the title Suggestion: wrap compound "&" #{&} selectors on sass to avoid invalid sourcemaps on libsass Suggestion: wrap compound "&" #{&} with interpolation to avoid invalid sourcemaps on libsass Jan 12, 2018
@acdvorak
Copy link
Contributor

Thanks for filing this issue! And thanks for including links to relevant issues/code and a sample repo - it's very helpful 😄

This may be related to #720.

I see no problem with your workaround. Feel free to send us a PR to fix it! 😀

@leocaseiro
Copy link
Author

leocaseiro commented Jan 19, 2018

Hi @acdvorak. Thanks for getting back to me.

I definitely believe it's related to #720. I'm sorry I didn't notice it. I'm just not sure if my PR will fix it as well, but I'm happy to investigate further.

I've sent the PR #1949 that fix at least my issue. In case it doesn't work for #720, we might need to interpolate more &'s

@leocaseiro
Copy link
Author

leocaseiro commented Jan 29, 2018

I just had the same issue on another project.

Any owner/contributor could please code review the PR #1949 ?

@acdvorak @traviskaufman @acdvorak @kfranqueiro

Cheers!

@asyncLiz
Copy link
Contributor

Closing since mdc-theme-dark has been removed

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

Successfully merging a pull request may close this issue.

3 participants