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

getLocalIdent function no longer falls back to default implementation #1191

Closed
tvsbrent opened this issue Sep 22, 2020 · 11 comments
Closed

getLocalIdent function no longer falls back to default implementation #1191

tvsbrent opened this issue Sep 22, 2020 · 11 comments

Comments

@tvsbrent
Copy link
Contributor

tvsbrent commented Sep 22, 2020

  • Operating System: MacOS 10.15.6
  • Node Version: 12.18.0
  • NPM Version: 6.14.5
  • webpack Version: 4.44.2
  • css-loader Version: 4.3.0

Expected Behavior

A getLocalIdent function that return a falsy value should fallback to the defaultGetLocalIdent function found in the utils.js.

Actual Behavior

With the 4.x release of css-loader, the behavior introduced in this pull request no longer works. A custom getLocalIdent function can no longer return a falsy value and have the default getLocalIdent function invoked.

I couldn't see any other convenient work arounds, like being able to import that default function and invoking it myself in my custom function.

Code

{
  test: /\.css$/,
  use: [
    {
      loader: 'style-loader'
    },
    {
      loader: 'css-loader',
      options: {
        modules: {
          exportLocalsConvention: 'camelCase',
          getLocalIdent: () => '',
          localIdentName: '[local]--[hash:base64:5]'
        }
      }
    }
  ]
}

How Do We Reproduce?

Have a getLocalIdent function return a falsy value.

@alexander-akait
Copy link
Member

It is breaking change getLocalIdent should always return string

@alexander-akait
Copy link
Member

Do not use getLocalIdent if you don't need custom generated local idents

@tvsbrent
Copy link
Contributor Author

tvsbrent commented Sep 22, 2020

Do not use getLocalIdent if you don't need custom generated local idents

I did not include the entire function, as to not bloat the issue report. I will say that I do need custom behavior in certain cases, for certain imports, but not always. Can you provide a work around so that I could still access the default getLocalIdent function? Could it just be exported and something I could pull into my custom function?

Alternatively, could having the custom implementation return an empty string be considered for triggering the old behavior that was lost?

@alexander-akait
Copy link
Member

alexander-akait commented Sep 22, 2020

No alternatives you should use own function or don't use the getLocalIdent option, in future we will escape returned values from getLocalIdent to avoid break CSS, so empty string will be valid as selector (we escape it)

@alexander-akait
Copy link
Member

alexander-akait commented Sep 22, 2020

If you found place where the default function doesn't generate valid CSS classname, please report about it and we will fix it

@tvsbrent
Copy link
Contributor Author

If you found place where the default function doesn't generate valid CSS classname, please report about it and we will fix it

Nope, that wasn't the case. In this case, we converted an internal library that previously bundled its CSS with very specific class selector names to start shipping that library as a "loose" library, with unbundled CSS files.

To provide more details, we were using the getLocalIdent function to test if the current file was part of that previously bundled library and, if so, use a very specific naming pattern for the selectors. This was to preserve any behaviors consumers of the library were doing that was dependent on those class selectors being named in that fashion. Otherwise, if the currently processed file wasn't a special one, we were falling back to the default ident function.

I think we could accomplish the same behavior with additional rules in the webpack config, it was just very convenient to bundle all that logic into one rule.

Anyway, thanks for the additional details.

@alexander-akait
Copy link
Member

@tvsbrent What is real use case? Why you need so complex logic?

@tvsbrent
Copy link
Contributor Author

We have 2 internal libraries that we consume in our frontend applications. One is a design system library, consisting of styled buttons and components like that. The other is a business-logic component library, one that has components most FE applications need, consisting of components like the user settings menu, log out, things like that.

Previously, we would bundle all the code and css up and have a consuming project use those bundles. In the bundled CSS we would add a very specific prefix to the selectors to namespace them. So button-input would become design-system-button-input. That very specific naming of the selectors was to allow the consuming app to override certain styles if they wanted.

Recently, we introduced tree shaking, so we started shipping unbundled sources in the packages. So now the consuming app is bundling all the CSS and generating the class selector names in its build. To preserve that old behavior of prefixing the design-system / business-logic library class selectors, we provide some Webpack config and utility functions. One such utility function was a custom getLocalIdent method. I've pasted a slightly anonymized version of that function below.

const getLocalIdent = (context, localIdentName, localName) => {
  if (shouldProcessAsDesignSystem(context.resourcePath)) {
    if (context.resourcePath.includes('base.css')) {
      // If using DS as a source library, we need to mangle the base.css names for backward compatibility
      if (sourceLibraries.includes('design-system')) {
        return `design-system-${localName}`;
      }
      // If using the packaged version, the names are already mangled appropriately
      return false;
    }
    // All other DS CSS uses this pattern
    return `design-system-${path.parse(context.resourcePath).name}-${localName}`;
  }
  if (shouldProcessAsBusinessLogic(context.resourcePath)) {
    return `business-logic-${path.parse(context.resourcePath).name}-${localName}`;
  }
  return false;
};

As you can see, we were using the falsy behavior the loader used to support to fall through to the default getLocalIdent function.

I can't say this is the most awesome thing we've ever done, but sometimes you go to production with the code you have! ;)

@alexander-akait
Copy link
Member

hm, okay, let's mark it as features, feel free to send a PR

@alexander-akait
Copy link
Member

Just interesting case what we should do with true value

@alexander-akait
Copy link
Member

Fixed, release will be in near future, we wait PostCSS 8 bugfix with RootExit and visitors

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

No branches or pull requests

2 participants