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

feat: add getJSON option to output CSS modules mapping #1577

Merged

Conversation

stephenkao
Copy link
Contributor

This PR contains a:

  • new feature
  • test update

Motivation / Use-Case

This changeset adds a getJSON option to output CSS modules mappings to JSON. This value can be a boolean or a function, and it employs similar logic to postcss-modules#getJSON as a function. This is particularly useful for SSR/SSG/templating languages when CSS modules mappings need to be present at build time.

Addresses #988.

Breaking Changes

N/A

Additional Info

Copy link

linux-foundation-easycla bot commented Mar 10, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

callback(error);

return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please run it only when option is true

src/utils.js Outdated

if (getJSON === true) {
// If true, output a JSON CSS modules mapping file in the same directory as the resource
await fsp.writeFile(outputPath, JSON.stringify(json));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid it, we can't cover all possible cases, so function is better solution

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, do you mean only allowing the a function value for getJSON and removing the true option for it? If so, I can definitely do that!

@stephenkao
Copy link
Contributor Author

@alexander-akait Would you mind taking another look when you get a chance? I've pushed an update here that removes the getJSON: true logic in favor of just getJSON: (resourcePath, json) => { ... } to simplify our assumptions. Thank you!

README.md Outdated
@@ -327,9 +327,7 @@ type modules =
| "dashesOnly"
| ((name: string) => string);
exportOnlyLocals: boolean;
getJSON:
| string
| ((resourcePath: string, json: object, outputPath: string) => any);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed outputPath as an argument here since that was an assumption made with getJSON: true. We shouldn't assume what the output path should be so we can just provide the user with actually relevant information.

src/utils.js Outdated
@@ -1413,26 +1412,6 @@ function syntaxErrorFactory(error) {
return obj;
}

async function writeModulesMap(getJSON, resourcePath, exports) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this function since the logic now is pretty simple in src/index.js. Happy to keep it as a simple function in src/utils.js if preferred, though!

@@ -2419,27 +2419,21 @@ describe('"modules" option', () => {
expect(getErrors(stats)).toMatchSnapshot("errors");
});

it("should output a co-located CSS modules map file with getJSON as true", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋

);

const fs = require("fs");
fs.writeFileSync(outputPath, JSON.stringify(json));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's improve the example and show how to write it in the one file, because it is a prefered way instead a lot of fs read calls

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With structure like:

{
  "resource-path.css": {
    "foo": "abcdef0987654321"
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note - additional example and keep this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, that definitely makes sense to me, but I'm not sure how to determine when we're "done" to write out the aggregated file. Like it's trivial to keep track of a Map/object in memory and add to it as getJSON is called, but it wouldn't know when to write out since each invocation of the loader function occurs in the context of an individual resource. Is there something like a lifecycle hook or a callback exposed to Webpack loaders like with plugins?

(Apologies if this is a dumb question--this is the first time I've worked on a Webpack loader. Thanks for all your guidance in any case!)

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and I am fine with, let's just improve examples

@stephenkao
Copy link
Contributor Author

stephenkao commented Mar 13, 2024

Working on fixing the failing tests--looks like I need to update some snapshots. Sorry about that!

Resolved in 0cdd520!

@@ -171,7 +171,7 @@
"type": "boolean"
},
"getJSON": {
"description": "Output CSS modules mapping through a callback.",
"description": "Allows outputting of CSS modules mapping through a callback.",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small update, just following conventions of existing language.

@stephenkao
Copy link
Contributor Author

stephenkao commented Mar 15, 2024

Ah, I also realized that composes won't work properly with this change since those are dynamically replaced during import. Like rather than returning something like:

{
  "foo": "foo_12345 foo_67890"
}

It'll return something like:

{
  "foo": "foo_12345 ___CSS_LOADER_ICSS_IMPORT_0___"
}

I still think this functionality is useful without composes support, but I'll keep thinking on it 🤔 Let me know if you have any suggestions, though!

@alexander-akait
Copy link
Member

@stephenkao ___CSS_LOADER_ICSS_IMPORT_0___ can be solved inside getJSON function, you just need traverse all keys and replace value, we need to show it in the example

@alexander-akait
Copy link
Member

Just rewrite ___CSS_LOADER_ICSS_IMPORT_XXX___ on ___CSS_LOADER_ICSS_IMPORT_XXX___[import-from-file], so when you get resourcePath and [import-from-file] the same, you just need to composes these classes, it doesn't sound difficult

@stephenkao
Copy link
Contributor Author

stephenkao commented Mar 20, 2024

Just rewrite ___CSS_LOADER_ICSS_IMPORT_XXX___ on ___CSS_LOADER_ICSS_IMPORT_XXX___[import-from-file], so when you get resourcePath and [import-from-file] the same, you just need to composes these classes, it doesn't sound difficult

Hope you've had a good weekend! And sorry for the delay--I've been out of town for a bit, and I just got back to working on this. I've taken your suggestion and got a working example of this in my project's codebase:

// CSS modules --> JSON helpers
const replacementMappings = {};
const allMappings = {};
const mappingHelper = {};
const CSS_LOADER_REPLACEMENT_REGEX =
  /___CSS_LOADER_ICSS_IMPORT_\d+_REPLACEMENT_\d+___/;
const REPLACEMENT_REGEX = /___REPLACEMENT\[(.*?)\]\[(.*?)\]___/;

function updateReplacementMappings(
  resourcePath,
  replacements,
  exportsJson,
  imports
) {
  // iterate through the replacements to generate standin variables
  replacementMappings[resourcePath] = replacements.reduce(
    (acc, { replacementName, importName, localName }) => {
      const replacementImportUrl = imports.find(
        importData => importData.importName === importName
      ).url;
      const relativePathRe = /.*!(.*)"/;
      const [, relativePath] = replacementImportUrl.match(relativePathRe);
      const importPath = path.resolve(path.dirname(resourcePath), relativePath);

      return {
        ...acc,
        [replacementName]: `___REPLACEMENT[${importPath}][${localName}]___`
      };
    },
    {}
  );

  // iterate through the raw exports and add standin variables
  // ('___REPLACEMENT[<absolute_path>][<class_name>]___')
  // to be replaced when absolute_path is actually read in by the loader
  Object.entries(exportsJson).forEach(([key, classNames]) => {
    exportsJson[key] = classNames
      .split(' ')
      .map(className => {
        if (CSS_LOADER_REPLACEMENT_REGEX.test(className)) {
          console.log(className);
          return replacementMappings[resourcePath][className];
        }

        return className;
      })
      .join(' ');
  });
}

function replaceStandIns(input) {
  // populate values in mappingHelper
  // (used as a quick lookup in replaceStrings)
  for (const key in input) {
    for (const subKey in input[key]) {
      const matches = input[key][subKey].match(REPLACEMENT_REGEX);
      if (matches) {
        const [_, resourcePath, property] = matches;

        mappingHelper[resourcePath] = mappingHelper[resourcePath] || {};

        if (!mappingHelper[resourcePath][property]) {
          mappingHelper[resourcePath][property] = input[resourcePath]
            ? input[resourcePath][property]
            : '';
        }
      }
    }
  }

  // recursively replace values
  function replaceStrings(obj) {
    for (const key in obj) {
      if (typeof obj[key] === 'string') {
        // replace CSS module mapping standin strings
        // '___REPLACEMENT[/foo/bar/baz.css][header]___' --> 'I1XbyWvimGjPnCLkhZ9Z'
        // if there are multiple depths of replacements,
        let match;
        while ((match = obj[key].match(REPLACEMENT_REGEX))) {
          const [_, resourcePath, property] = match;
          if (
            mappingHelper[resourcePath] &&
            mappingHelper[resourcePath][property]
          ) {
            // value in mappingHelper exists--
            // just populate with it and continue
            obj[key] = obj[key].replace(
              `___REPLACEMENT[${resourcePath}][${property}]___`,
              mappingHelper[resourcePath][property]
            );
          } else {
            // canonical value in mappingHelper does not exist yet--
            // it'll be populated on a future loader call,
            // but break for now to preserve standin strings to be replaced later
            break;
          }
        }
      } else if (typeof obj[key] === 'object') {
        // object --> go through each kv pair and replace CSS module mapping standin strings
        replaceStrings(obj[key]);
      }
    }
  }

  replaceStrings(input);

  return input;
}

...

// Webpack config
{
  loader: 'css-loader',
  options: {
    modules: {
      // TODO: I'd need to update the css-loader getJSON call signature
      getJSON: ({
        resourcePath,
        exportsJson,
        replacements,
        imports
      }) => {
        updateReplacementMappings(
          resourcePath,
          replacements,
          exportsJson,
          imports
        );

        allMappings[resourcePath] = exportsJson;

        replaceStandIns(allMappings);
        console.log({ allMappings });   
      }
    }
  }
}

This replaces all import strings like ___CSS_LOADER_IMPORT_x_REPLACEMENT_y___ with ___REPLACEMENT[<resource_path>][<key>]___, and it recursively tries to replace the latter when we have the mappings available to us, given the loaded resource.

allMappings at the end of this would look like:

{
  "/root/first.module.css": {
    "a": "I1XbyWvimGjPnCLkhZ9Z", // or more depending on composition like "I1XbyWvimGjPnCLkhZ9Z u5tAUi4dTEY3O3IWmwQ5"
    "b": "D2OyZ8aIiNqG9WycMKWF"
  },
  "/root/second.module.css": {
    "c": "u5tAUi4dTEY3O3IWmwQ5",
  }
}

However, it's still expensive and annoyingly complex since it's iterating over each value to determine whether or not we have to replace the 'REPLACEMENT[x][y]' values in each CSS module mapping--we don't have all mappings available to us since the loader function's called on each file individually so we're building on the helper objects on each call. Is there a way to determine once all loaded files have been loaded (at least during a build rather than in watch mode)? That would help to cut down on the redundant calls, and it'd give us a definite time to write out the final .json file like you requested here.

Thank you for all of your help! I very naïvely thought this would be a simple thing, but I'm super grateful for your guidance!

@alexander-akait
Copy link
Member

Is there a way to determine once all loaded files have been loaded (at least during a build rather than in watch mode)?

No

I very naïvely thought this would be a simple thing, but I'm super grateful for your guidance!

That is why it was not implemented 😄

Anyway we can improve our logic and keep strings only with CSS_LOADER_REPLACEMENT_REGEX in memory (in Map for example) for replacing and then traverse only them (i.e. keep position (keys) in file how we can fast to get access to avoid traverse all keys/values), so we will not run it for simple values, I would like to say that your solution is working, it just needs optimization.

Another solution is just add a simple plugin:

{
  apply(compiler) {
    compier.hooks.done.tap("collect", () => {
      // run logic here
    });
  }
}

But I think it is possible to solve without a plugin

@stephenkao
Copy link
Contributor Author

stephenkao commented Mar 21, 2024

That is why it was not implemented 😄

Had I only known that I was signing myself up for... 🤣🤣🤣

Anyway we can improve our logic and keep strings only with CSS_LOADER_REPLACEMENT_REGEX in memory (in Map for example) for replacing and then traverse only them (i.e. keep position (keys) in file how we can fast to get access to avoid traverse all keys/values), so we will not run it for simple values, I would like to say that your solution is working, it just needs optimization.

Another solution is just add a simple plugin:

{
  apply(compiler) {
    compier.hooks.done.tap("collect", () => {
      // run logic here
    });
  }
}

But I think it is possible to solve without a plugin

Ohh sure, that makes sense! I think it'd help to simplify the logic a lot if we did use a plugin, though. Like the loader could just create a map of all CSS module mappings with stand-in variables, and the plugin would resolve all stand-in variables with their canonical values and output the final .json file (kind of like how MiniCssExtractPlugin works in practice?). That way, we wouldn't have to worry about load orders since all possible values would be loaded in memory by the time the plugin's called. I'll give it a try and report back!

@stephenkao
Copy link
Contributor Author

@alexander-akait Hokay, I pushed up an update with an example that uses a plugin to reconcile the import replacements. It splits off canonical values and uses them verbatim while it keeps track of replacements to recursively replace them. Replace replace replace replacements 😅

Would you mind taking a look whenever you get a chance? (I know I sound like a broken record at this point, but thank you so much for your help!)

src/utils.js Outdated
@@ -593,6 +593,8 @@ function getModulesOptions(rawOptions, exportType, loaderContext) {
? "camelCaseOnly"
: "asIs",
exportOnlyLocals: false,
// eslint-disable-next-line no-undefined
getJSON: undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just avoid it, because we already check it in our code in undefined

},
},
],
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep only this, because other developers can use another solution and faced with the problem better avoid it for better DX

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And can you add a test case with your solution and put it in helpers directory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-akait Sure, I can definitely add a test case!

Regarding the README changes, which parts are you suggesting we keep versus remove? I'm happy to update whatever, but I'm not sure which part you're referring to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean keep only part where we replace ___REPLACEMENT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, do you mean removing the other two examples that write to files on each loader call (here and here) and only keeping the third example in its entirety or omitting parts of the third example? The third example has a lot more logic involved than I would have liked, but I'm not sure it'd make sense without both the loader and plugin parts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The third example has a lot more logic involved than I would have liked, but I'm not sure it'd make sense without both the loader and plugin parts.

Let's keep it, composes is a popular things, so better developer will use it, maybe someone send a PR with some optimizations in future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! Thanks for your understanding 🙏

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay, looks good, let's do small improvements and we can merge, thank you

@alexander-akait
Copy link
Member

And let's fix tests CI tests

@@ -1,6 +1,6 @@
import stripAnsi from "strip-ansi";

function removeCWD(str) {
export function removeCWD(str) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm exporting this to strip out the cwd from the getJSON snapshots.

@stephenkao
Copy link
Contributor Author

@alexander-akait I've updated the tests and snapshots so they should be working now: I collapsed the two test cases into one since they were fundamentally testing the same thing, and I'm reusing removeCWD to make sure the resourcePaths don't differ between environments.

Let me know what you think whenever you get a chance! Thank you very much!

@alexander-akait
Copy link
Member

@stephenkao Thank you, I want to make release now, because we need some changes to be align with webpack built-in CSS future and rspack, anyway I am glad to see your improvement, so please rebase and I will release it in 7.1.0 (when you just finish this PR) 😄 Sorry for delay in some things, a lot of issue 😞

This changeset adds a `getJSON` option to output CSS modules mappings to JSON.
This value can be a boolean or a function, and it employs similar logic to
[postcss-modules#getJSON](https://github.com/madyankin/postcss-modules?tab=readme-ov-file#saving-exported-classes) as a function.
This is particularly useful for SSR/SSG/templating languages when CSS modules mappings need to be present at build time.

Addresses [webpack-contrib#988](webpack-contrib#988).
@stephenkao
Copy link
Contributor Author

@alexander-akait No worries, I totally understand! I just rebased on top of latest master and pushed up the changes so everything should be up-to-date now 🙏

alexander-akait
alexander-akait previously approved these changes Apr 8, 2024
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 97.19%. Comparing base (f5be49c) to head (cd3b7ae).
Report is 18 commits behind head on master.

Files Patch % Lines
src/index.js 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1577      +/-   ##
==========================================
+ Coverage   96.15%   97.19%   +1.04%     
==========================================
  Files          10       10              
  Lines        1196     1178      -18     
  Branches      461      449      -12     
==========================================
- Hits         1150     1145       -5     
+ Misses         37       29       -8     
+ Partials        9        4       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stephenkao
Copy link
Contributor Author

stephenkao commented Apr 8, 2024

@alexander-akait I saw that there was a lint issue with a redefined url variable in url-option.test.js so I renamed the local variable here. I don't think this was related to my changeset so I can definitely revert that if you'd like!

EDIT: One more time, sorry! I needed to run Prettier on my latest changes 😅

@alexander-akait
Copy link
Member

Ignore codecov, I will fix it before release, thank you, release will be soon

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

Successfully merging this pull request may close these issues.

2 participants