[Theming] theme.spacing doesn't accept multiple string arguments

[zettca]

Using a custom function for theme.spacing with multiple arguments (theme.spacing("xs", "sm")) used to work in v4.8, now it doesn't.

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Given a custom spacing function like:

const customSpacing = key => {
  const mySpacings = { xs: 10, sm: 30, lg: 50 };
  return mySpacings[key] || key;
};

And applying it like so: padding: theme.spacing("sm", "xs")

The output is: padding: sm xs

Expected Behavior 🤔

Expected the output to be padding: 30px 10px

Steps to Reproduce 🕹

CodeSandbox of the example above:
https://codesandbox.io/s/material-demo-sv3h3?file=/demo.js:430-455

If you change to "@material-ui/core": "~4.8.0" it works fine

Steps:

1. Notice how the div padding isn't properly applied (inspect the red div element)

Context 🔦

Your Environment 🌎

Tech	Version
Material-UI	v4.10.0
React
Browser
TypeScript
etc.

[oliviertassinari]

We had this effort that conflict with your use case: #20408. However, it seems that we didn't approach is correctly. What do you think of this fix?

diff --git a/packages/material-ui-system/src/spacing.js b/packages/material-ui-system/src/spacing.js
index 41d191eb8..27b20fc1e 100644
--- a/packages/material-ui-system/src/spacing.js
+++ b/packages/material-ui-system/src/spacing.js
@@ -79,6 +79,10 @@ export function createUnarySpacing(theme) {

   if (typeof themeSpacing === 'number') {
     return (abs) => {
+      if (typeof abs === 'string') {
+        return abs;
+      }
+
       if (process.env.NODE_ENV !== 'production') {
         if (typeof abs !== 'number') {
           console.error(`Material-UI: Expected spacing argument to be a number, got ${abs}.`);
diff --git a/packages/material-ui/src/styles/createSpacing.js b/packages/material-ui/src/styles/createSpacing.js
index 779a8d2ae..66a344b75 100644
--- a/packages/material-ui/src/styles/createSpacing.js
+++ b/packages/material-ui/src/styles/createSpacing.js
@@ -34,9 +34,6 @@ export default function createSpacing(spacingInput = 8) {

     return args
       .map((argument) => {
-        if (typeof argument === 'string') {
-          return argument;
-        }
         const output = transform(argument);
         return typeof output === 'number' ? `${output}px` : output;
       })
diff --git a/packages/material-ui/src/styles/createSpacing.test.js b/packages/material-ui/src/styles/createSpacing.test.js
index dc29b6289..9eabc0403 100644
--- a/packages/material-ui/src/styles/createSpacing.test.js
+++ b/packages/material-ui/src/styles/createSpacing.test.js
@@ -42,7 +42,12 @@ describe('createSpacing', () => {
     let spacing;
     spacing = createSpacing();
     expect(spacing(1, 'auto')).to.equal('8px auto');
-    spacing = createSpacing((factor) => `${0.25 * factor}rem`);
+    spacing = createSpacing((factor) => {
+      if (typeof factor === 'string') {
+        return factor;
+      }
+      return `${0.25 * factor}rem`
+    });
     expect(spacing(1, 'auto', 2, 3)).to.equal('0.25rem auto 0.5rem 0.75rem');
   });

Do you want to work on it? (and add a new test case to avoid regressions? :)

[eps1lon]

@oliviertassinari Your test implies a breaking change.

[oliviertassinari]

@eps1lon The elements that could support it's a bug fix for a regression rather than a breaking change:

• The test case we break in question comes from [theme] Support string args in theme.spacing #20408. It's not directly linked to the issue we were trying to solve.

    spacing = createSpacing((factor) => `${0.25 * factor}rem`);
    expect(spacing(1, 'auto', 2, 3)).to.equal('0.25rem auto 0.5rem 0.75rem');

• The only required test for [theme] Support string args in theme.spacing #20408, that match the linked issue, was:

const spacing = createSpacing();
expect(spacing(1, 'auto')).to.equal('8px auto');

• We should have had a test case for @zettca's use case before making [theme] Support string args in theme.spacing #20408, to not introduce this regression

[eps1lon]

createSpacing((factor) => ${0.25 * factor}rem);

Why did we have a test if this was never supported? Please don't include internals in test or explicitly mark them as private to enable the whole team to work on the code.

[oliviertassinari]

@eps1lon Yeah, agree, this might not have been the wisest decision, it was meant to create a stronger constraint, to better explore the scope of the customization options available. Should we wait for v5 to change it back?

[zettca]

As I user, I expected that by using a custom function, its result wouldn't be "hijacked" in some special cases.
Our actual spacing function is something like this:

const spacing => key => {
  const mySpacings = { xs: 10, sm: 30, lg: 50 };
  switch (typeof key) {
    case "string":
      return mySpacings[key] || key;
    case "number":
      return 10 * key || 0;
    default:
      return 0;
  }
};

With something like this you can support factor spacing, "xs"/"sm"/"lg" spacing, as well as any other string (auto, unset), etc. Can't we support every use case like this?

Anyways, just my two cents on the matter.

[oliviertassinari]

As I user, I expected that by using a custom function, its result wouldn't be "hijacked" in some special cases

@zettca Thanks for sharing more details on how you use the spacing function. I think that we all agree that it's the desired direction to go into. The question at stake is: how?

[GuilleDF]

I can implement @oliviertassinari's proposed solution, if that's okay with you guys 😃