fix(commonjs): dynamic require root check was broken in some cases#1461
fix(commonjs): dynamic require root check was broken in some cases#1461shellscape merged 6 commits intorollup:masterfrom
Conversation
|
Thanks for the PR. We won't be able to accept this without accompanying tests for your fix. |
|
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. |
|
Thanks for having a look at the tests, but it looks like some are still failing due to outdated snapshots.
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. |
Using the normalized path in _all_ the properties of the error
|
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.
I am working on it right now, I intend to publish a new variant of the test, with intentionally conflicting path syntaxes. |
|
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 |
|
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. |
Rollup Plugin Name:
commonjsThis PR contains:
Are tests included?
Breaking Changes?
Description
When using the
dynamicRequireTargetsoption, the files detected by the option are matched against the dynamic require root path used by the plugin — either the project root or thedynamicRequireRootoption to ensure that the root contains all dynamically required paths.The test is done in
packages/commonjs/index.js:142through a simple string comparison usingid.indexOf(dynamicRequireRoot).In some cases, it happens that
dynamicRequireRootis using the OS path syntax (e.g. backslashes on Windows) butidis 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 bothidanddynamicRequireRoothave 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?