postcss / postcss-simple-vars

PostCSS plugin for Sass-like variables
MIT License
415 stars 36 forks source link

add option to allow to warn on undefined variables without throwing #26

Closed ashelley closed 9 years ago

ashelley commented 9 years ago

This commit adds an option to warn when you hit an undefined variable. This patch continues processing the css similarly to {silent: true} but also adds a warning message. This is useful to provide feedback for linting purposes.

This patch is unnecessarily ugly because node.warn() doesn't seem to exist as per:

https://github.com/postcss/postcss/blob/master/docs/api.md#nodewarnresult-message

I didn't investigate the source cause of why the convenience method didn't exist on the node and instead changed the method signatures to pass around "result" to facilitate calling result.warn directly.

It'd probably be worth investigating why the node does not have the warn method instead of accepting this patch verbatim. Maybe I just missed something.

Let me know what you think about this functionality.

ai commented 9 years ago

@ashelley here we have some complicated philosophy issue. Can you show me some user case, when warning is better than error in this case?

ashelley commented 9 years ago

Hello @ai,

Currently I am using the {silent: true} option. This enables the CSS to still compile without replacing the variable so that the majority of the css in the file still works. This is the behavior I like.

However, I would like to report on the fact that undefined variables are being used to provide feed back to the user (linting).

Currently there is no way for the user to know why one specific variable usage isn't working due to a spelling mistake and still have most of the css still work.

ai commented 9 years ago

@ashelley can you explain why you use silent: true? Maybe with some examples?

ashelley commented 9 years ago

Hello @ai,

Sorry for the delayed response. I'm a tiny bit pressed for time at the current moment to setup a working codepen but what I am doing is using this mode (along with my modifications) to live update css on a webpage and also put gutter errors on a code mirror instance like this:

https://codemirror.net/demo/lint.html

The reason for safe: true is so that the live update still works except for rules using undefined variables.

ai commented 9 years ago

What you mean in “live update”?

My point is that most of programming languages will throw a error on unknow variable. Because unknon variable in sources is a error.

So maybe we can fix your issue in other way? Or maybe I am wrong and there is some user case for it.

ashelley commented 9 years ago

If I get a chance i'll setup a demo! Not sure when this will be. For now I can just work off my fork so no worries!

ai commented 9 years ago

@ashelley fork is not a best way :). Remember what problem has JS because of missed var variables. This is why next JS do not allow it.

So I think undefined variables is a evil in long term.

ai commented 9 years ago

Or you eant to create some syntax checker?

ashelley commented 9 years ago

It's for updating the code mirror linting as per the above link. However I want it to work like just an invalid value for a property instead of blowing up completely.

ai commented 9 years ago

What do you think about custom callback for undefined variables? I do not want to promote silent warning mode.

ashelley commented 9 years ago

To be honest any thing that reports the line and column number of the undefined variables would be fine for me. It would be a bit more work to write code to deal with regular postcss warnings and errors on another code path but nothing is impossible.

Sort of off topic:

Right now i'm relying on a quirk to inspect which variables are defined by relying on this behavior:

https://github.com/postcss/postcss-simple-vars/blob/master/index.js#L64

When you pass a variables as a function it uses a reference to your variable instead of creating a new variables object so I rely on this this quirk to find out which variables are defined in a css string:

var variables = {};
vars({
    variables: function() {
        return variables;
    },
    silent: true
});

// now i know what variables are defined in this file!

If there was an explicit way to get variable declarations/definitions out of the file as well as get the line,column, and name of variable usages that would be great.

I could even compare the list myself to decide if they were used by not defined. That would be just fine with me.

ai commented 9 years ago

Yeap, we can add some APIs. I will think about API after PostCSS 5.0 release.

ai commented 9 years ago

I added unknown callback 8a9f342

ai commented 9 years ago

Sorry, right now I have no more explicit way to return variables :(.

ashelley commented 9 years ago

@ai thanks, this looks good and doesn't seem to break the in-explicit way of getting variables :)