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

Recursive sort #50

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Recursive sort #50

wants to merge 2 commits into from

Conversation

vis97c
Copy link

@vis97c vis97c commented Jun 1, 2023

Following the disscusions on #38 and OlehDutchenko/sort-css-media-queries#24 I decided to implement some sort of feature to adress it. Feel free to comment and let me know what is needed for this to be merged. The code is not complete (missing tests coverage) but it works. I wanted to open the PR because I was also working on an aditional feature to nest the media queries conditionally.

In implementing the changes mainly because I'm trying to ship the smallest possible css using stable enough features. Merging the atRules and media queries has reduced 30% of the file size already and I'm pushing for another significant reduction after I'm able to implement the media querie nesting. I'm having problems rn but I've aready ran out of time to figure it out, so probably next week I'll work on it.

Again I'll await your comments.

@vis97c
Copy link
Author

vis97c commented Jun 1, 2023

image

Here are the benchmarks with the additional nest feature.

My setup:

// postcss.config.js
module.exports = {
  map: false,
  plugins: [
    // Optimizations
    require("postcss-preset-env")({
      stage: 4,
      features: {
        "nesting-rules": { noIsPseudoSelector: true },
      },
      minimumVendorImplementations: 3,
      autoprefixer: false,
    }),
    require("cssnano")(nano),
    require("postcss-merge-selectors")({}),
    require("postcss-sort-media-queries")({
      sort: "desktop-first",
      onlyTopLevel: true,
      mergeAtRules: false,
      nested: false,
      configuration: {
        unitlessMqAlwaysFirst: true,
      },
    }),
    require("postcss-precision")({}),
  ],
};

@yunusga
Copy link
Owner

yunusga commented Jun 5, 2023

Hi, @vis97c I think it is necessary to divide this functionality into 2 different plugins because additional complexity is added with different types of atRules. Nedd new plugin postcss-merge-atrules

@vis97c
Copy link
Author

vis97c commented Jun 5, 2023

Hi, @vis97c I think it is necessary to divide this functionality into 2 different plugins because additional complexity is added with different types of atRules. Nedd new plugin postcss-merge-atrules

Fair enough. I will split the code then.

@vis97c
Copy link
Author

vis97c commented Jun 6, 2023

@yunusga Do you think this module should stick to sorting without actually merging? Because with the second module that functionality will be duplicated if we take into account the current behavior of the plugin. Merge & sort.

I'm thinking for this module: Sorting recursively but do not merge.
New module: Merge & optionally nest, but do not sort.

So the usage will look like this:

// postcss.config.js
module.exports = {
  map: false,
  plugins: [
    // Sort media querie like at rules (@media, @container)
    require("postcss-sort-media-queries")({
      sort: "desktop-first",
      onlyTopLevel: true,
      mqRegex: /(media|container)/im, // under consideration as well
      configuration: {
        unitlessMqAlwaysFirst: true,
      },
    }),
    // Merge at rules, preserving the previous order
    require("postcss-merge-at-rules")({
      atRuleRegex: /(media|container|layer|scope|supports)/im,
      nest: true,
    }),
  ],
};

@vis97c
Copy link
Author

vis97c commented Jun 12, 2023

@yunusga as promised, I just created the package: postcss-merge-at-rules. Don't worry I will be also be updating this merge request with the propper changes (recursive sorting mostly). I hope we could get to an agreement on the before mention concerns before that.

@yunusga
Copy link
Owner

yunusga commented Jun 12, 2023

Hi @vis97c, sorry, there was a lot of work, I will try to solve the issue with the update today or tomorrow

@vis97c vis97c changed the title Merge recursively and add mergeAtRules option to merge aditional atRules Recursive sort Jun 14, 2023
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.

None yet

2 participants