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

Function names over-ride css properties. #2516

Closed
hybridwebdev opened this issue Dec 11, 2019 · 6 comments
Closed

Function names over-ride css properties. #2516

hybridwebdev opened this issue Dec 11, 2019 · 6 comments

Comments

@hybridwebdev
Copy link

hybridwebdev commented Dec 11, 2019

Consider the following generic .styl sheet

generic.styl

font-size()
	color #f00
.some-css-class 
	font-size: 20px

What is happening is font-size no longer functions as a css property within the compiled sheet, instead it invokes the function call. What you end up with is:

generic.css

.some-css-class {
   color: #f00
}

So as you can see, not only is the property declaration ignored, it is ignored and invokes the function call instead. While I admit the fallacy of declaring functions with the same name as properties, this is a really weird side effect.

This stung me because I was using flex declarations in the same sheets where I had a flex() function.

@vendethiel
Copy link
Contributor

Working as intended

@lightbringer1991
Copy link

maybe changing your function name since it's too confusing for a new developer to look at the code.

@hybridwebdev
Copy link
Author

hybridwebdev commented Dec 12, 2019

Working as intended

I'm not sure how that's intentional? In my example the font-size within the css rule isn't even a function call. Note that it's: "font-size" not "font-size()" meaning the declaration SHOULD be interpreted as a property declaration, not a function call. I think this should be assumed as property names SHOULD take precedence due to how css rules work.

As for function name ambiguity, i do agree I should have named my function better, no arguments.

However, anyone using compiled css languages like stylus, sass etc aren't newbs. They clearly understand basic css concepts, otherwise they wouldn't be using an advanced system like compiled css languages. In that instance, i highly doubt the coder isn't going to be able to tell the difference between something intended as a function call, vs a property declaration.

My point remains, this issue clearly illustrates an oversight/bug. To dismiss this with a statement of "just name your functions better" Is missing the point entirely. In my case, I am a highly skilled full stack dev well versed in a wide range of programming languages (both front and backend) and this particular issue still caused me a lot of grief because:

a) This caveat is not documented in stylus'es documentation.
b) With several .styl files that compiled into upwards of 10k+ lines of css, this was real tough to troubleshoot as I had css being applied and no clue where it was coming from.

Whilst I understand that solutions to this may be limited (eg you can't just enforce a strict rule where function calls MUST have the closure as this would introduce breaking changes), it might be a good idea to potentially add a compiler warning stating the issue with ambiguity, as well as update the documentation to reflect this caveat.

@MoussaAdam
Copy link

MoussaAdam commented Dec 19, 2019

yeah it works as intended, this is stylus' "transparent mixins" feature, its stated in the docs, but not under functions section, its under mixins section, bc your font-size() is not a function, its a mixin

  • functions return values
  • mixins apply css declarations

even if you don't look under mixins section, it's on the home page
official-stylus-website
consider that:

border-radius()
  -webkit-border-radius: arguments
  -moz-border-radius: arguments
  border-radius: arguments
  //prevent childrens from showing thier corners
  overflow: hidden

button
  border-radius: 12px

this is a useful feature

i mean... the only reason i can think of to give your mixin the same identifier as a css property, is to override it, (maybe for example to add vendor specific properties)

i think overriding is a very expected behavior for most languages, the most sane solution is to change your function name to avoid confusion for the readers and for the stylus transpiler

can you please close the issue

BTW: i agree that its unusual feature and its expected to cause confusion, i mean its not that elegant, and its not ... an expected language design, but its helpful

i think changing the syntax to prefix mixins will be a very good decision, maybe start them with @

this should be added to #2104

@hybridwebdev
Copy link
Author

hybridwebdev commented Jan 15, 2020

No, sorry I won't close it, as this is an important issue that needs to be addressed. Leaving this ticket open may help others who stumble across this issue, until it's resolved.

I do like the idea of strictly enforcing prefixing mixins/functions/variables with some sort of declaration (eg: $) to ensure this sort of tom-foolery doesn't result.

Sure, overriding is a common practice in modern programming languages. However in this context, arbitrarily re-assigning the behavior of a css property because of a preceding function declaration is NOT the same thing. In this case, it's confusing and silly.

Running with the idea of prefixing variables/functions with $ would ensure more consistent/expected behavior in this particular example eg: the following just makes far more sense.

$font-size()
   ... stuffs

.some-class
   font-size 20px
   $font-size

Meaning users literally can never shoot themselves in the foot by "accidentally" over-riding property behaviors. This is called defensive programming, and whilst i understand users hands shouldn't have to be fully held and be allowed flexibility, in this particular case it's a sanity check that should exist.

@groenroos
Copy link
Collaborator

No, sorry I won't close it, as this is an important issue that needs to be addressed. Leaving this ticket open may help others who stumble across this issue, until it's resolved.

I think this issue should be closed. As others have noted, it is a documented headline feature working as intended - I'm not sure any resolution is possible here without breaking loads of existing Stylus projects. Others with the same problem can still find this issue, even if it's closed.

I do like the idea of strictly enforcing prefixing mixins/functions/variables with some sort of declaration (eg: $) to ensure this sort of tom-foolery doesn't result.

Your best bet to enforce your personal preference of coding style (incl. mixin naming convention) is a linter, such as stylint. For instance, you might find their prefixVarsWithDollar and colons rules useful.

cc: @iChenLei

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

No branches or pull requests

6 participants