Skip to content

Conversation

teazean
Copy link

@teazean teazean commented Dec 12, 2017

ℹ️ I've set options.sourceMap to false explicitly, but I still get warnings from dev console.

Type


ℹ️ What types of changes does your code introduce?

Put an x in the boxes that apply

  • Fix
  • Perf
  • Docs
  • Test
  • Chore
  • Feature
  • Refactor

SemVer


ℹ️ What changes to the current semver range does your PR introduce?

Put an x in the boxes that apply

  • Bug (:label: Patch)
  • Feature (:label: Minor)
  • Breaking Change (:label: Major)

Issues


ℹ️ What issue (if any) are closed by your PR?

Replace #1 with the error number that applies

  • Fixes #1

Checklist


ℹ️ You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is a reminder of what we are going to look for before merging your code.

Put an x in the boxes that apply.

  • Lint and unit tests pass with my changes
  • I have added tests that prove my fix is effective/works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes are merged and published in downstream modules

@coveralls
Copy link

coveralls commented Dec 12, 2017

Coverage Status

Coverage remained the same at 86.842% when pulling b2b65f9 on teazean:master into b9c1add on postcss:master.

@alexander-akait
Copy link
Member

@teazean can you describe use case for this?

@teazean
Copy link
Author

teazean commented Dec 12, 2017

@evilebottnawi

When I was developing, I was confused by the warning tip。

Previous source map found, but options.sourceMap isn't set.

But I have already set sourceMap to false, I suspected some configuration is still wrong. It wasted a lot of time finding out why I got a warning.

I think warning tip should be more precise.

@alexander-akait
Copy link
Member

@teazean this mean previous loader return source map, you should disable source map on all loaders, this message allow to track this problem (i.e. you previous loader generate source map but other loader after postcss don't get source map, you just spend build time on generate source maps but you don't use)

@teazean
Copy link
Author

teazean commented Dec 12, 2017

@evilebottnawi
yes, but maybe we can choose one from below which could be more precise:

  1. change options.sourceMap isn't set to options.sourceMap is a falsy value
  2. change if (!sourceMap to if (sourceMap === undefined

By the way, if I set options.sourceMap to false clearly, I will know what's the result, and then I think the warning tip is unnecessary.

@michael-ciniawsky
Copy link
Member

@teazean This warning is mainly displayed if you have a previous source map (map, e.g from the sass-loader) and options.sourceMap = undefined || false || null in postcss-loader. If the following 2 configs still work

webpack.config,js

// 1
{
   test: /\.scss$/.
   use: [
      ...,
      { loader: 'postcss-loader', options: {} }, // warning
      { loader: 'sass-loader', options: { sourceMap: true } }
   ]
},
// 2
{
   test: /\.scss$/.
   use: [
      ...,
      { loader: 'postcss-loader', options: { sourceMap: false } }, // warning
      { loader: 'sass-loader', options: { sourceMap: true } } // if false/unset => no warning
   ]
}

but the main (your) issue is that currently explicitly setting sourceMap = false is a false negative that triggers the warning I will accept this PR. Otherwise I'm afraid have to declined it.

@michael-ciniawsky michael-ciniawsky changed the title optimize sourceMap warning tip fix(index): check sourceMap === undefined for warning display (options.sourceMap) Dec 13, 2017
@michael-ciniawsky michael-ciniawsky added this to the 2.0.10 milestone Dec 13, 2017
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide your current webpack.config.js aswell

@teazean
Copy link
Author

teazean commented Dec 13, 2017

@michael-ciniawsky Here is my warning tip.
image

And below is my webpack config for vue-loader options.

{
  "loaders": {
    "css": [
      "vue-style-loader",
      {
        "loader": "css-loader",
        "options": {
          "minimize": false,
          "sourceMap": false
        }
      },
      {
        "loader": "postcss-loader",
        "options": {
          "sourceMap": false
        }
      }
    ],
    "postcss": [
      "vue-style-loader",
      {
        "loader": "css-loader",
        "options": {
          "minimize": false,
          "sourceMap": false
        }
      },
      {
        "loader": "postcss-loader",
        "options": {
          "sourceMap": false
        }
      }
    ]
  },
  "transformToRequire": {
    "video": "src",
    "source": "src",
    "img": "src",
    "image": "xlink:href"
  }
}

The file compiled with warning is a Vue file with no style#lang configed

@teazean
Copy link
Author

teazean commented Dec 13, 2017

@michael-ciniawsky
I mean that the waring tip should not be displayed when postcss-loader#sourceMap is set to false explicitly.

Or we can change the tip, not using not set(I set the postcss-loader#sourceMap already, and why do I get a not set warning?).

@michael-ciniawsky
Copy link
Member

@teazean If the vue-loader generates a source map this warning will still appear and rightfully so then, could you open an issue about this in vuejs/vue-loader aswell and verify if/why vue-loader adds a map of some kind here ?

@teazean
Copy link
Author

teazean commented Dec 13, 2017

@michael-ciniawsky
Can we just change the warning tip from options.sourceMap isn't set to options.sourceMap is a falsy value, since it actually cost a lot of time finding out why I get a warning options.sourceMap isn't set when I set options.sourceMap to false explicitly.

@Justineo
Copy link

undefined → isn't set
falsy value → is disabled

I think it's just a wording issue for the warning message.

@michael-ciniawsky
Copy link
Member

I'm of course open to improve the warning message if it's misleading/confusing :)

@teazean
Copy link
Author

teazean commented Dec 13, 2017

@michael-ciniawsky
Updating warning is enough for my case. I'm closing this PR, and thanks for your explanation.👍

Looking forward to your warning improvement.

@michael-ciniawsky michael-ciniawsky removed this from the 2.0.10 milestone Dec 13, 2017
@teazean
Copy link
Author

teazean commented Dec 13, 2017

😞 Sorry for reopening this PR, and I will push the improved warning.

@teazean teazean reopened this Dec 13, 2017
@coveralls
Copy link

coveralls commented Dec 13, 2017

Coverage Status

Coverage remained the same at 86.842% when pulling a617b46 on teazean:master into b9c1add on postcss:master.

@michael-ciniawsky michael-ciniawsky added this to the 2.0.10 milestone Dec 13, 2017
@michael-ciniawsky michael-ciniawsky changed the title fix(index): check sourceMap === undefined for warning display (options.sourceMap) refactor(index): improve sourceMap warning message (options.sourceMap) Dec 13, 2017
@michael-ciniawsky michael-ciniawsky removed this from the 2.0.11 milestone Feb 2, 2018
@michael-ciniawsky michael-ciniawsky removed the request for review from alexander-akait February 2, 2018 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants