Tighten webpack rule merge rules to prevent clashes

[AntoLepejian]

Problem

Playroom has its own webpack config, and has the ability for consumers to provide their own, which are both merged to create a single webpack config.

The merge matches rules, i.e. determines their uniqueness only by comparing the test property of a rule. So if a consumer tries to add a module rule with the same test as one of playroom's own configs, they will be treated as equal and thus one will override the other.

e.g.

const playroomConfig = {
  {
    test: /\.css$/,
    include: path.dirname(require.resolve('codemirror/package.json')),
    use: [MiniCssExtractPlugin.loader, require.resolve('css-loader')],
  },
}

const consumerConfig = {
  {
    test: /\.css$/,
    include: /my_files/,
    use: "style-loader",
  },
}

In the above example, both rules are considered equal, so one will take precedence, leading to a misconfiguration.

Solution

Include the include and exclude properties in the comparison too, to ensure a rule is uniquely identified by all 3 properties test, include and exclude.

[changeset-bot]

🦋 Changeset detected

Latest commit: 31f06c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
playroom	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[AntoLepejian]

Hi @askoufis 👋,

You seem the be the most active maintainer, so was wondering if you could have a look at my small bug fix PRs 🙏🏼

[askoufis]

Thanks for the PR!

[askoufis]

@AntoLepejian It seems your fork is a bit restrictive and won't run CI checks.

[AntoLepejian]

@AntoLepejian It seems your fork is a bit restrictive and won't run CI checks.

@askoufis interesting. I have no idea why, lemme check

[AntoLepejian]

@askoufis I think you have to approve it?

"This workflow requires approval from a maintainer. Learn more about approving workflows.
2 expected checks"

[askoufis]

@askoufis I think you have to approve it?

"This workflow requires approval from a maintainer. Learn more about approving workflows.
2 expected checks"

Oh my bad, I think I approved it before but then merged my suggestion so I think it got cancelled or something.

[askoufis]

Hey @AntoLepejian, I'm going to revert this change. It has broken our internal usage of playroom, and the workaround (without reverting this change) isn't very practical. In our case, because we can no longer easily override playroom's CSS handling, we end up with .css files loaded twice, leading to errors.

From digging up a bit of context around the config merging feature, I think keeping the merge scoped to just the test was intentional so consumers could easily override playroom's behaviour. The feature was always intended as an escape hatch, with consumers needing to either BYO loaders for various files, or re-implement playrooms loaders if they want to scope their loaders to just their files.

Happy to chat more about your specific use case so we can reach a solution that works for everyone, but until then I'll need to revert this change.

[AntoLepejian]

Hey @AntoLepejian, I'm going to revert this change. It has broken our internal usage of playroom, and the workaround (without reverting this change) isn't very practical. In our case, because we can no longer easily override playroom's CSS handling, we end up with .css files loaded twice, leading to errors.

From digging up a bit of context around the config merging feature, I think keeping the merge scoped to just the test was intentional so consumers could easily override playroom's behaviour. The feature was always intended as an escape hatch, with consumers needing to either BYO loaders for various files, or re-implement playrooms loaders if they want to scope their loaders to just their files.

Happy to chat more about your specific use case so we can reach a solution that works for everyone, but until then I'll need to revert this change.

@askoufis thanks for getting back to me with an explanation!

Obviously if this has broken your internal usage of playroom, then we need to revert it 😂

I was tackling this from a consumer's POV, where I'd want to BYO my loaders without impacting playroom's ones whatsoever; and vice versa, by creating a clear separation boundary. So to me your workaround (which was deemed impractical) semantically made sense. But I can see arguments for both.

I understand you'd want to override the config, so I'm happy to also try recreating playroom's loaders when I override the CSS loader, which means I may not need this PR to land, thanks for the suggestion!

The only action needed might be to communicate this via JSDocs, or in the readme :)

[AntoLepejian]

@askoufis so i've come up with a workaround to unblock myself.

I need to add separate CSS loaders for separate files, and now that I understand the problem, I can bypass it.

One of my css rules is now .+\.css$ and the other .*\.css$. These (almost) equivalent regex matchers mean I can do so without my CSS loaders getting overridden by playroom's.

Not pretty, but will do for now 🙂