fix: fix support for function name strings for Sass < 3.5 by ncoden · Pull Request #117 · foundation/motion-ui

ncoden · 2018-06-03T21:58:33Z

In order to improve modular namespacing, Sass 4 will only accepts first-class functions as argument for call() so functions will be called in their own context. As a first step, Sass 3.5 added get-function and throw a warning if a function name string is passed to call().

In #100, we started to use get-function() for all calls to call() to remove the warning and prepare support for Sass 4. This broke Motion UI for all Sass versions < 3.5 as they do not support get-function().

This PR fixes the support for Sass < 3.5 by introducing a set of helpers to safely manage both first-class functions and function name strings.

Note: In most case, get-function() must only be used by the user in its own context. End developer must be encouraged to use first-class functions.

Changes:

Add a set of helpers to test, validate and call function and function name stings in a safe way for all Sass versions.
Use -mui-is-function instead of a simple string type check as functions can now be first-hand functions with a function type
Use -mui-safe-call to handle both first-hand functions and function string names the same way. An error is thrown if the given argument is not callable, with informations about get-function() if relevant.

Added helpers:

-mui-is-function($value)
Return if a given value is a function or a function name string.
-mui-is-callable($value)
Return if a given value is callable.
-mui-assert-function($value)
Check if a given value is callable and throw the appropriate error otherwise
-mui-safe-get-function($func)
Return a reference to the given function or function name string compatible with the current Sass version. For Sass < 3.5, return the passed argument. For Sass >= 3.5, return a first-class function if a function name string was passed.
-mui-safe-call($func, $args...)
Polyfill for the call function supporting both functions and strings.

References:

Sass 3 incompatibility introduced in Resolves Passing a string to call() deprecation warning for Sass 4.0 #100
See http://oddbird.net/2017/03/30/safe-get
See http://sass.logdown.com/posts/809572-sass-35-release-candidate

…ass 4 In order to improve modular namespacing, LibSass 4 only accepts first-class functions as argument so functions are called in their own context. In most case, `get-function()` must only be used by the user in its own context. End developer must be encouraged to use first-class functions. In this library, we need to use it to keep support of function name strings in Sass > 3.5. Changes: * Add `-mui-is-function` and `-mui-safe-call` to test and call function in a safe way for all Sass versions. * Use `-mui-is-function` instead of a simple `string` type check as functions can now be first-hand functions with a `function` type * Use `-mui-safe-call` to handle both first-hand functions and function string names the same way. A warning is thrown for function string names with Sass >= 3.5

…ents

`function-exists()` can only take strings and cannot be called for first-class functions.

…not found Instead of warning about the usage of function name strings in Sass >= 3.5, check for function arguments and throw the appropriate error if invalid or not found.

ncoden · 2018-06-03T22:42:43Z

Poke @gakimball and @DanielRuf for review. 👋

KittyGiraudel · 2018-06-04T07:32:42Z

src/util/_series.scss

 /// @param {Duration} $duration [1s] - Length of the animation.
 /// @param {Duration} $gap [0s] - Amount of time to pause before playing the animation after this one. Use a negative value to make the next effect overlap with the current one.
-/// @param {Function} $keyframes... - One or more effect functions to build the keyframe with.
+/// @param {Function|String} $keyframes... - One or more effect functions to build the keyframe with.


This parameter is technically an arglist given you use .... I believe the correct notation would be {Function[]|String[]}?

SassDoc doesn't mention that notation or anything for .... But you built that tool so I'll believe you 😄

@hugogiraudel hmmm... Shouldn't it be {Arglist} actually ? I don't find any reference or usage for the [] notation.

KittyGiraudel · 2018-06-04T07:33:59Z

src/util/_function.scss

+/// @return {*}
+///
+@function -mui-safe-call($func, $args...) {
+  $_: -mui-assert-function($func);


If you want to make this code a little less odd, you could write it like this:

@function -mui-safe-call($func, $args...) { @if (-mui-assert-function($func)) { @return call(-mui-safe-get-function($func), $args...); } }

This is an assert function. We expect it to throw an error if anything is wrong. I don't think this is odd, but I'll make the change if you think it is really needed.

I understand what it does. The fact is, you use an $_ because Sass doesn’t allow calling functions outside of a variable or an expression. That’s a hack. A necessary one, admittedly.

What I believe your code means is “if the function is callable, call it”. Hence why I would wrap it in an @if block. It produces the same result and appears to be slightly more clean to me.

I don’t feel strongly about it, it’s just a comment.

Alright, it seems legit. I'll do that 👍

KittyGiraudel · 2018-06-04T07:34:40Z

src/util/_function.scss

+/// @return {Boolean}
+///
+@function -mui-assert-function($value) {
+  @if -mui-is-callable($value) == true {


You could omit the == true here. :)

ncoden · 2018-06-04T07:41:52Z

@hugogiraudel Thank you for the review 👍. Do you have an opinion about the approach itself ? Could it be used for SassyList ?

KittyGiraudel · 2018-06-04T07:54:20Z

I suppose so? I’m not really invested into SassyLists anymore though. :)

ncoden · 2018-06-04T20:00:58Z

@hugogiraudel I took care of the requested changes. I used Arglist for all argument lists as I found nothing about the [] notation.

KittyGiraudel · 2018-06-05T07:31:24Z

@ncoden SassDoc does not maintain a list of types, so feel free to use the notation you want. Arglist is good because it is genuinely a valid Sass type, however it doesn’t give much information about the type of elements in the arglist. You could probably use the description for that.

ncoden · 2018-06-05T07:35:23Z

You could probably use the description for that.

True, I checked for that. We already always precise the type in the description

ncoden added 5 commits June 3, 2018 22:26

docs: add missing support of function name strings for function argum…

15353d9

…ents

fix: fix check -mui-is-function check for first-class functions

152dcf2

`function-exists()` can only take strings and cannot be called for first-class functions.

fix: add missing util/function import

54ad939

refactor: replace function string deprecation warning by an error if …

2eb8d33

…not found Instead of warning about the usage of function name strings in Sass >= 3.5, check for function arguments and throw the appropriate error if invalid or not found.

ncoden changed the title ~~[WIP] fix: fix Sass 3 support for function string names~~ fix: fix support for function name strings for Sass < 3.5 Jun 3, 2018

ncoden added sass bug fix labels Jun 3, 2018

ncoden requested a review from gakimball June 3, 2018 22:42

ncoden mentioned this pull request Jun 3, 2018

fix: fix libsass 4 call deprecation warning #63 at-import/SassyLists#65

Open

KittyGiraudel reviewed Jun 4, 2018

View reviewed changes

ncoden added 2 commits June 4, 2018 21:52

refactor: simplify and remove hacks from function helpers

b69a83c

docs: use SassDoc "Arglist" type for argument lists

fb3fc61

ncoden mentioned this pull request Jun 5, 2018

🚀 Release v2.0.0 #113

Merged

4 tasks

rediris approved these changes Jun 5, 2018

View reviewed changes

ncoden merged commit 716be70 into foundation:develop Jun 5, 2018

Conversation

ncoden commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Added helpers:

References:

Uh oh!

ncoden commented Jun 3, 2018

Uh oh!

KittyGiraudel Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

ncoden Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoden Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

KittyGiraudel Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

ncoden Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KittyGiraudel Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

ncoden Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KittyGiraudel Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

ncoden commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KittyGiraudel commented Jun 4, 2018

Uh oh!

ncoden commented Jun 4, 2018

Uh oh!

KittyGiraudel commented Jun 5, 2018

Uh oh!

ncoden commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ncoden commented Jun 3, 2018 •

edited

Loading

ncoden Jun 4, 2018 •

edited

Loading

ncoden Jun 4, 2018 •

edited

Loading

ncoden Jun 4, 2018 •

edited

Loading

ncoden commented Jun 4, 2018 •

edited

Loading

ncoden commented Jun 5, 2018 •

edited

Loading