fix(commonjs): dynamic require root check was broken in some cases

[Elarcis]

Rollup Plugin Name: commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (updated existing tests for the feature)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

Description

When using the dynamicRequireTargets option, the files detected by the option are matched against the dynamic require root path used by the plugin — either the project root or the dynamicRequireRoot option to ensure that the root contains all dynamically required paths.

The test is done in packages/commonjs/index.js:142 through a simple string comparison using id.indexOf(dynamicRequireRoot).

In some cases, it happens that dynamicRequireRoot is using the OS path syntax (e.g. backslashes on Windows) but id is using unix paths (regular slashes). This leads to an issue where even if the root is correct, a simple string comparison fails to recognise that.

I reused the normalizePathSlashes() function provided in the plugin’s src to ensure that both id and dynamicRequireRoot have similar slash logic so that the check doesn’t fail.

Please advise on how to automate a test for that fix, as I have 0 experience on writing tests for Rollup plugins and that behaviour is located deep within the source, in a lambda returned by a function returned by another function with lots of params. Would exporting that lambda in a public function do the trick?

[shellscape]

Thanks for the PR. We won't be able to accept this without accompanying tests for your fix.

[Elarcis]

I’ve just received a notification that a previously existing test failed to pass with my fix, which seems both accurate given the check made by the test, and surprising because running the tests locally didn’t raise the issue.

Will fix tomorrow.

[shellscape]

Thanks for having a look at the tests, but it looks like some are still failing due to outdated snapshots.

We won't be able to accept this without accompanying tests for your fix.

What we're looking for in your PR here is a test that specifically takes in intentionally broken parameters and still passes. That will protect us from regressions if someone submits a fix that reverts your changes in some way, down the road.

[Elarcis]

Hi, I believe I fixed the failing test. It was pretty hard to understand why the tests are failing in CI, but not on my WSL setup, and I’m still not sure why.

What we're looking for in your PR here is a test that specifically takes in intentionally broken parameters and still passes. That will protect us from regressions if someone submits a fix that reverts your changes in some way, down the road.

I am working on it right now, I intend to publish a new variant of the test, with intentionally conflicting path syntaxes.

[Elarcis]

The test should work. I was able to trigger the erroneous error from the plugin parameters, and to ensure that my fix makes that error disappear.

However, the test only “pollutes” part of the value, ensuring that the path must be sanitised for the check to be correct. It is not strictly the same issue as on Windows per how path works, but it essentially requires the same fix and should prevent any accidental rollback.

[shellscape]

There's a few bits that could constitute a breaking change, so we're going to release this as a new major. Nothing special, just precautionary.